feat: add multi-organization support#275
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds first-class multi-organization support: Organization model, migrations and factories, per-organization pivot roles plus a global super_admin flag, CurrentOrganization service and middleware, OrganizationScope, UI for managing/switching orgs, authorization updates, and pervasive scoping/wiring of resources and tests to an active organization context. ChangesMulti-Tenant Organization System
Sequence Diagram(s)sequenceDiagram
participant Req as HTTP Request
participant MW as SetCurrentOrganization
participant Current as CurrentOrganization
participant Org as Organization
Req->>MW: handle($request)
MW->>Current: resolveForUser(user, cookie/org_id/header)
Current->>Org: load or fallback to Main
MW->>Req: allow or abort based on resolved org
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #275 +/- ##
============================================
+ Coverage 91.32% 91.62% +0.30%
- Complexity 2179 2328 +149
============================================
Files 192 203 +11
Lines 7818 8278 +460
============================================
+ Hits 7140 7585 +445
- Misses 678 693 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (11)
app/Livewire/Dashboard/JobStatusGrid.php (1)
68-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSame cross-org data leakage as
LatestJobs:selectedJobqueries without org scoping.
jobs()is correctly scoped viaforCurrentOrg(), but theselectedJobcomputed property (line 75) usesBackupJob::with(...)->find($this->selectedJobId)with no org restriction. Combined with the unlockedpublic ?string $selectedJobId(line 18) and the publicly callableviewLogs(string $id)action, an authenticated user can read job detail data from another organization.🔒 Proposed fix
+use Livewire\Attributes\Locked; public bool $showLogsModal = false; +#[Locked] public ?string $selectedJobId = null; // ... #[Computed] public function selectedJob(): ?BackupJob { if (! $this->selectedJobId) { return null; } - return BackupJob::with([ + return BackupJob::forCurrentOrg()->with([ 'snapshot.databaseServer', 'snapshot.triggeredBy', 'restore.snapshot.databaseServer', 'restore.targetServer', 'restore.triggeredBy', ])->find($this->selectedJobId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Dashboard/JobStatusGrid.php` around lines 68 - 82, The computed property selectedJob uses BackupJob::with(...)->find($this->selectedJobId) with no organization scoping, allowing cross-org data access; update selectedJob to apply the same forCurrentOrg() scope used by jobs() (e.g. BackupJob::forCurrentOrg()->with(...)->find(...)) and/or validate that $this->selectedJobId belongs to the current org before returning, and ensure viewLogs and the public property selectedJobId cannot bypass this check (reference selectedJob, BackupJob::with, forCurrentOrg(), selectedJobId, and viewLogs).tests/Feature/Mcp/McpServerTest.php (1)
188-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::first()->idwithout null safety risks aTypeErrorif no org is seeded.Both lines 195 and 258 call
Organization::first()->iddirectly. If no organization exists at test execution time,Organization::first()returnsnulland PHP throws aTypeError. The factory (line 28 ofDatabaseServerSshConfigFactory.php) consistently uses the null-safeOrganization::first()?->id— the test should match. UsingOrganization::main()->id(or at minimum the null-safe operator /firstOrFail()) is more explicit and resilient.🛡️ Proposed fix
- 'organization_id' => \App\Models\Organization::first()->id, + 'organization_id' => \App\Models\Organization::main()->id,Apply the same change at both line 195 and line 258.
Also applies to: 248-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Mcp/McpServerTest.php` around lines 188 - 196, The test uses Organization::first()->id which can throw a TypeError when no Organization exists; update both occurrences (the DatabaseServer creation at the block using \App\Models\DatabaseServer::create and the other call around line 258) to use a null-safe or explicit main/fail approach (e.g., Organization::first()?->id or Organization::main()->id or Organization::firstOrFail()->id) so the test no longer assumes a non-null Organization and mirrors the factory's null-safe use; ensure both instances are changed consistently.docs/docs/user-guide/permissions.md (1)
51-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winContradiction between the Users permissions table and the User Deletion Restrictions section.
Line 56 shows that an Admin can "Delete user" (✅), but lines 63–66 state "Only super admins can delete users." If "Delete user" in the table means remove from organization and the super-admin restriction applies to permanently deleting an account, the documentation should use different labels to make this distinction explicit (e.g., "Remove from organization" vs. "Delete account"). As written, the two sections directly conflict.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/user-guide/permissions.md` around lines 51 - 66, The permissions table entry "Delete user" conflicts with the "User Deletion Restrictions" section; update the docs to disambiguate by renaming the table action and/or the restriction header: change the table action "Delete user" to "Remove from organization" (keeping Admin = ✅) and add a new row "Permanently delete account" with Viewer = ❌ Member = ❌ Admin = ❌ Super Admin = ✅, and update the "User Deletion Restrictions" heading to explicitly reference "Permanently delete account" (and keep the two bullets about self-deletion and last-super-admin restrictions) so the table and the special rules are consistent.app/Livewire/Dashboard/LatestJobs.php (1)
58-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCross-org data leakage:
getSelectedJobProperty()fetches jobs without org scoping.
fetchJobs()is now correctly scoped toforCurrentOrg(), butgetSelectedJobProperty()uses a plainBackupJob::with(...)->find($this->selectedJobId). Any public method in a Livewire component can be called from the client-side, even without an associated wire:click handler — users can still trigger the action from the browser's DevTools. SinceviewLogs(string $id)accepts any string from the client and sets$this->selectedJobId, andselectedJobIdis also an unprotected public property that should be treated as un-trusted input from the user, an authenticated user in Org A can read job logs (including eager-loaded snapshot and database server data) belonging to Org B.Add org scoping to the lookup and add
#[Locked]onselectedJobIdas defence-in-depth:🔒 Proposed fix
+use Livewire\Attributes\Locked; public bool $showLogsModal = false; +#[Locked] public ?string $selectedJobId = null; // ... public function getSelectedJobProperty(): ?BackupJob { if (! $this->selectedJobId) { return null; } - return BackupJob::with([ + return BackupJob::forCurrentOrg()->with([ 'snapshot.databaseServer', 'snapshot.triggeredBy', 'restore.snapshot.databaseServer', 'restore.targetServer', 'restore.triggeredBy', ])->find($this->selectedJobId); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Dashboard/LatestJobs.php` around lines 58 - 71, getSelectedJobProperty currently loads a BackupJob by id without org scoping and selectedJobId is unprotected; update getSelectedJobProperty to use the org scope (e.g. BackupJob::forCurrentOrg()->with(...)->find($this->selectedJobId)) and return null if not found, and add the #[Locked] attribute to the selectedJobId property declaration to treat it as untrusted input (also ensure viewLogs uses the same guarded property). This ensures lookups are constrained to the current org and selectedJobId is hardened as defence-in-depth.tests/Feature/DatabaseServer/SshTunnelTest.php (1)
16-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Volume::factory()->create()instead of rawVolume::create(), and guard against a nullOrganization::first().Two issues appear in all four test cases:
Coding guideline:
Volume::create([...])bypasses the factory. TheVolumeFactoryalready defaultsorganization_idcorrectly (Organization::first()?->id ?? Organization::factory()->main()), so switching to the factory removes the duplication and keeps tests consistent.Potential NPE:
Organization::first()->idhas no null-safe guard. If migrations don't seed a main org (e.g. in an isolated run), this throws aTypeError. The factories and other test helpers use?->idor a fallback — these tests should do the same.🛠️ Proposed fix (shown for the first occurrence; apply identically to lines 65-70, 109-114, and 191-196)
- $volume = Volume::create([ - 'name' => 'Test Volume', - 'type' => 'local', - 'config' => ['path' => '/var/backups'], - 'organization_id' => \App\Models\Organization::first()->id, - ]); + $volume = Volume::factory()->create();As per coding guidelines: "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
Also applies to: 65-70, 109-114, 191-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/SshTunnelTest.php` around lines 16 - 21, Replace raw Volume::create([...]) calls with Volume::factory()->create() so the VolumeFactory defaults (including organization_id) are used; if you must set organization_id explicitly, guard against a null Organization by using Organization::first()?->id ?? Organization::factory()->main() (or create()) instead of Organization::first()->id. Update the occurrences around the Volume creation in SshTunnelTest (the lines using Volume::create and Organization::first()) and apply the same change to the other three blocks mentioned.tests/Feature/User/IndexTest.php (1)
59-78:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winThe test body never asserts the actual protection it is named for.
The test "admin cannot delete the last admin" only proves a non-final admin can be deleted. After deleting
$anotherAdminand returning to one super admin, the test exits without attempting to delete$this->adminand verifying that the action is forbidden. The comment on line 77 even acknowledges the gap.✅ Proposed completion of the test
expect(User::find($anotherAdmin->id))->toBeNull(); - // Now try to delete the last admin - should fail on authorize - // We need to create another user to try from (but they need to be admin) - // Actually, the test should verify you can't delete when count = 1 + // Create a second super admin to act as the deleter, then attempt + // to delete the only remaining super admin — must be rejected. + $secondAdmin = User::factory()->superAdmin()->create(); + Livewire::actingAs($secondAdmin) + ->test(Index::class) + ->call('confirmDelete', $this->admin->id) + ->assertForbidden(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/User/IndexTest.php` around lines 59 - 78, The test never actually attempts to delete the final super admin; after removing $anotherAdmin it should try to delete $this->admin using the same Livewire flow and assert the action is forbidden. Update the "admin cannot delete the last admin" test to, after deleting $anotherAdmin, call Livewire::test(Index::class)->call('confirmDelete', $this->admin->id)->call('delete') (or the equivalent flow used by Index::class), then assert the deletion was prevented by checking User::find($this->admin->id) is not null and by asserting the Livewire component signaled failure (e.g., unauthorized/forbidden event, validation error, or emitted error message) so the test verifies confirmDelete/delete on $this->admin is rejected.app/Queries/BackupJobQuery.php (1)
25-49:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd org scoping to
make()to fix data isolation breach in API endpoint.
make()returns unscoped queries that include BackupJobs from all organizations.BackupJobdoes not register a globalOrganizationScope—it only provides a localscopeForCurrentOrg()method that must be explicitly called. Meanwhile,buildFromParams()correctly callsforCurrentOrg()on line 70.The API endpoint
BackupJobController::index()callsBackupJobQuery::make()->paginate()directly without additional scoping, allowing callers to retrieve BackupJobs across all tenants. Add->forCurrentOrg()to the chain inmake().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Queries/BackupJobQuery.php` around lines 25 - 49, The make() query builder currently returns unscoped BackupJob::class results; to enforce tenant isolation add the model scope call forCurrentOrg() into the fluent chain inside BackupJobQuery::make() (the same scope exposed by BackupJob::scopeForCurrentOrg()/forCurrentOrg()), so the chain becomes QueryBuilder::for(BackupJob::class)->forCurrentOrg()->with(self::RELATIONSHIPS).... This mirrors buildFromParams() and prevents BackupJobController::index() from returning jobs across organizations.tests/Feature/Services/Backup/LocalVolumeStorageTest.php (1)
38-56:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Organization::main()instead ofOrganization::first()for consistency.Other test files in this PR (e.g.,
DemoModeTest.phpline 215,CreateTest.phpline 59) use\App\Models\Organization::main()to reference the main organization.Organization::first()is order-dependent and diverges from the established pattern.♻️ Proposed fix
- 'organization_id' => \App\Models\Organization::first()->id, + 'organization_id' => \App\Models\Organization::main()->id,Apply the same change to both occurrences (lines 42 and 55).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Services/Backup/LocalVolumeStorageTest.php` around lines 38 - 56, Replace the order-dependent Organization::first() calls used when creating test volumes with the canonical Organization::main() helper; specifically update the Organization lookup in the Volume::create calls that set 'organization_id' (the two occurrences near the Volume::create for $volumeWithRoot and $volumeWithPath) to use \App\Models\Organization::main()->id so the tests follow the established pattern.tests/Feature/DatabaseServer/EditTest.php (1)
14-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Organization::main()instead ofOrganization::first()for consistency.Same issue as
LocalVolumeStorageTest.php:Organization::first()is order-dependent and inconsistent withOrganization::main()used throughout the rest of this PR's test changes.♻️ Proposed fix
- 'organization_id' => \App\Models\Organization::first()->id, + 'organization_id' => \App\Models\Organization::main()->id,Apply to all four occurrences (lines 18, 25, 105, 116).
Also applies to: 22-26, 101-106, 109-117
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/EditTest.php` around lines 14 - 19, Replace uses of Organization::first()->id with Organization::main()->id in EditTest.php to ensure consistent, order-independent test data; specifically update the four occurrences that build Volume records (the Volume::create calls and any other places where organization_id is set using Organization::first()) so they call Organization::main()->id instead (search for Organization::first() in the file and swap to Organization::main()).app/Services/DemoBackupService.php (1)
14-63:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
DemoBackupService::createDemoBackup()will crash at runtime during first-user registration becauseCurrentOrganizationis never resolved.During registration, the
SetCurrentOrganizationmiddleware runs before the user exists, so it cannot resolve an organization.CreateNewUserthen creates the main organization and attaches the user, but never calls$currentOrganization->set()or$currentOrganization->switchTo()to set the resolved context. WhencreateDemoBackup()is subsequently called, it invokes$this->currentOrganization->id()on lines 42, 51, and 63, which dereferences$this->organization->idwithout a null-check, resulting in aTypeErrorbecause$organizationis stillnull.The registration test masks this crash by mocking the entire service via
$this->app->instance(), so the real code path is never executed.Fix: Call
$this->currentOrganization->set($mainOrg)after creating and attaching the user to the main organization, or add a guard check increateDemoBackup()before callingid().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/DemoBackupService.php` around lines 14 - 63, The createDemoBackup() method in DemoBackupService calls $this->currentOrganization->id() assuming CurrentOrganization is resolved, which can be null during first-user registration; fix by setting the current organization immediately after creating/attaching the main organization in the user-registration flow (e.g., in CreateNewUser after you create $mainOrg and attach the user, call CurrentOrganization::set($mainOrg) or switchTo($mainOrg)) so DemoBackupService::createDemoBackup() always sees a resolved organization, or alternatively add a null-guard in DemoBackupService::createDemoBackup() that throws a clear RuntimeException if CurrentOrganization has no organization set before calling id().tests/Feature/User/EditTest.php (1)
14-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse factory states for role assignment instead of manual attributes.
The factory properly handles the post-migration virtual
'role'attribute throughnewModel()interception andafterCreating()pivot attachment, so the tests will work. However, per the coding guideline to check for custom states before manually setting up models, replace direct attribute passing with the available factory states:->viewer(),->admin(),->demo(). For member role, omit the attribute entirely since it is the default.Lines 15, 24, 38, 90 and similar patterns should use:
User::factory()->viewer()->create()instead of->create(['role' => 'viewer']), andUser::factory()->create()instead of->create(['role' => 'member']).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/User/EditTest.php` around lines 14 - 39, Replace direct role attribute creation in the User factory calls with the provided factory states and defaults: use User::factory()->viewer()->create() (and ->admin() or ->demo() where appropriate) for explicit viewer/admin/demo users, and simply User::factory()->create() for default member users; apply this change in the tests named "non-admin users cannot access edit user page", "viewer cannot access edit user page", "admin can edit another user" (and any similar tests) so tests rely on the factory states/newModel interception and afterCreating pivot logic rather than manually passing ['role' => ...].
🟡 Minor comments (6)
config/oauth.php-15-25 (1)
15-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
OIDC_prefix is inconsistent with all other OAuth config env varsThe config comment says "auto-created OAuth/OIDC users" (covering Google, GitHub, GitLab, and OIDC alike), but the env variable is named
OIDC_DEFAULT_ORGANIZATION_ID. Every other option in this file uses theOAUTH_prefix. An operator configuring Google or GitHub OAuth will not expect to look for anOIDC_-prefixed variable to control organization assignment.🔧 Suggested rename
- 'default_organization_id' => env('OIDC_DEFAULT_ORGANIZATION_ID'), + 'default_organization_id' => env('OAUTH_DEFAULT_ORGANIZATION_ID'),Update
.env.exampleand the documentation page accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/oauth.php` around lines 15 - 25, The env var for the config key default_organization_id is incorrectly using the OIDC_ prefix (OIDC_DEFAULT_ORGANIZATION_ID) which is inconsistent with other options; change the env lookup to use OAUTH_DEFAULT_ORGANIZATION_ID in the config (default_organization_id => env('OAUTH_DEFAULT_ORGANIZATION_ID')), and update any related artifacts (e.g., .env.example and documentation) to reflect the new OAUTH_ name so Google/GitHub/GitLab operators can find the setting.tests/Feature/DatabaseServer/CreateTest.php-17-18 (1)
17-18:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::first()->idwill fatal-error if no Organization exists.Every
Volume::create()call uses\App\Models\Organization::first()->id(without the null-safe operator). If an Organization isn't in the database at the moment this line executes — due to test isolation, a changed seeding order, or a future test refactor — PHP will throw a fatal error trying to access->idonnull. TheDatabaseServerFactoryalready uses the defensive formOrganization::first()?->id ??.🛡️ Proposed fix (representative example — apply to all occurrences)
-'organization_id' => \App\Models\Organization::first()->id, +'organization_id' => \App\Models\Organization::first()?->id ?? \App\Models\Organization::factory()->main()->create()->id,Alternatively, extract a shared
mainOrganization()test helper (similar to the existingdailySchedule()/weeklySchedule()helpers visible in the file) to avoid repeating this across every test.Also applies to: 110-111, 149-150, 268-269, 296-296, 313-313, 330-330, 356-357, 433-434, 474-475, 480-481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/CreateTest.php` around lines 17 - 18, Replace unsafe Organization::first()->id usages in Volume::create() calls with a null-safe fallback to ensure tests don't fatal when no Organization exists; specifically update each occurrence of Organization::first()->id to use Organization::first()?->id ?? Organization::factory()->create()->id (or call a shared helper like mainOrganization() that returns an existing id), and mirror the defensive pattern already used in DatabaseServerFactory so all referenced locations (e.g., the Volume::create(...) calls) use the null-safe operator plus a create fallback.app/Services/CurrentOrganization.php-130-135 (1)
130-135:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::main()may return null;switchTo(null)will TypeError.If
main()returnsnull(e.g., the backfill migration failed to seed it, or a super_admin was created against a freshly-migrated schema where seeding didn't run),switchTo(Organization::main())violatesswitchTo(Organization $org). Same null-safety story applies if the main org row was manually deleted in production. A defensiveif ($main = Organization::main())plus log-or-throw on the else branch keeps the failure mode comprehensible.if ($user->isSuperAdmin()) { - $this->switchTo(Organization::main()); - - return; + $main = Organization::main(); + if ($main !== null) { + $this->switchTo($main); + } + + return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/CurrentOrganization.php` around lines 130 - 135, The call to switchTo(Organization::main()) can pass null and cause a TypeError; update the super-admin fallback in CurrentOrganization (the block using Organization::main() and switchTo(Organization $org)) to first assign and guard the result (e.g., if ($main = Organization::main()) { $this->switchTo($main); return; } else { /* log error or throw a clear exception */ }), ensuring you either log a descriptive error or throw a specific exception when main() returns null instead of calling switchTo with null.app/Livewire/Forms/UserForm.php-89-114 (1)
89-114:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReturning
falseon last-super-admin demotion swallows feedback.
update()short-circuits withreturn falsewhen the only super admin tries to demote themselves, but the form does not surface an error message to the user. Whether the calling Livewire component displays a meaningful failure depends on its boilerplate handling of the boolean. UseaddError()(or throw aValidationException) so the message reliably reaches the UI and the form state stays consistent.♻️ Proposed fix
if (auth()->user()->isSuperAdmin()) { // Cannot remove the last super admin if ($this->user->isSuperAdmin() && ! $this->superAdmin) { if (User::where('super_admin', true)->count() === 1) { - return false; + $this->addError('superAdmin', __('Cannot remove the last super admin.')); + return false; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Forms/UserForm.php` around lines 89 - 114, The code currently returns false when preventing the last super admin from being demoted (inside the auth()->user()->isSuperAdmin() branch checking $this->user->isSuperAdmin() && ! $this->superAdmin), which silently fails; replace that return false with a proper UI-facing error by calling $this->addError('super_admin', 'Cannot remove the last super admin') (or throw a ValidationException) so the Livewire form surfaces the message and halts further updates; keep the rest of the update logic (the $this->user->update(...) calls and the pivot updateExistingPivot on organizations()) unchanged and ensure you exit the method after adding the error.database/factories/UserFactory.php-64-75 (1)
64-75:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::first()may not be the main org.
afterCreatingattaches the user to whichever organization happens to come first by primary key, even if it is not flaggedis_main. In tests that pre-seed multiple orgs and rely on the user being a member of the main one (e.g. assertions usingroleIn(Organization::main())), this can quietly fail. Prefer querying the main org explicitly or fall back to creating one viaOrganizationFactory::main().♻️ Proposed fix
- $org = Organization::first() ?? Organization::factory()->main()->create(); + $org = Organization::where('is_main', true)->first() + ?? Organization::factory()->main()->create();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/factories/UserFactory.php` around lines 64 - 75, In configure()'s afterCreating closure, stop using Organization::first() and instead query for the main organization explicitly (e.g. Organization::where('is_main', true)->first() or Organization::main()->first()) and if none exists fall back to Organization::factory()->main()->create(); update the reference to Organization::first() in the closure to use this main-org lookup so tests that expect membership in the main org (and the factory method Organization::factory()->main()) reliably attach the user to the main organization.app/Models/Organization.php-104-112 (1)
104-112:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
hasResources()omitssshConfigsfrom the check, though cascade delete protects them.SSH configs are deleted when the organization is deleted (via
cascadeOnDelete()in the schema), so there's no data loss risk. However, since the method gates the delete button visibility, operators see "no resources" even when SSH configs exist. IncludesshConfigsin the check for clarity—so the UI accurately reflects all organization-scoped assets being removed.♻️ Proposed fix
public function hasResources(): bool { return $this->databaseServers()->exists() || $this->volumes()->exists() - || $this->agents()->exists(); + || $this->agents()->exists() + || $this->sshConfigs()->exists(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Organization.php` around lines 104 - 112, The hasResources() method currently checks databaseServers(), volumes(), and agents() but omits sshConfigs(), causing the delete UI to report no resources when SSH configs exist; update hasResources() to also include $this->sshConfigs()->exists() (i.e., return $this->databaseServers()->exists() || $this->volumes()->exists() || $this->agents()->exists() || $this->sshConfigs()->exists()) so the method reflects all organization-scoped assets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b9128dfa-4f65-464b-80df-856fde8fc0b6
📒 Files selected for processing (77)
app/Actions/Fortify/CreateNewUser.phpapp/Http/Controllers/Api/V1/DatabaseServerController.phpapp/Http/Controllers/Api/V1/VolumeController.phpapp/Http/Middleware/DemoModeMiddleware.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Dashboard/JobStatusGrid.phpapp/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/LatestJobs.phpapp/Livewire/Dashboard/SnapshotsCard.phpapp/Livewire/Dashboard/StorageCard.phpapp/Livewire/Dashboard/StorageDistributionChart.phpapp/Livewire/Dashboard/SuccessRateCard.phpapp/Livewire/Forms/AgentForm.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/Forms/VolumeForm.phpapp/Livewire/OrganizationSwitcher.phpapp/Livewire/User/Create.phpapp/Livewire/User/Edit.phpapp/Livewire/User/Index.phpapp/Models/Agent.phpapp/Models/BackupJob.phpapp/Models/DatabaseServer.phpapp/Models/DatabaseServerSshConfig.phpapp/Models/Organization.phpapp/Models/Scopes/OrganizationScope.phpapp/Models/Snapshot.phpapp/Models/User.phpapp/Models/Volume.phpapp/Policies/OrganizationPolicy.phpapp/Policies/UserPolicy.phpapp/Providers/AppServiceProvider.phpapp/Queries/BackupJobQuery.phpapp/Queries/SnapshotQuery.phpapp/Services/CurrentOrganization.phpapp/Services/DemoBackupService.phpapp/Services/OAuthService.phpbootstrap/app.phpconfig/oauth.phpdatabase/factories/AgentFactory.phpdatabase/factories/DatabaseServerFactory.phpdatabase/factories/DatabaseServerSshConfigFactory.phpdatabase/factories/OrganizationFactory.phpdatabase/factories/UserFactory.phpdatabase/factories/VolumeFactory.phpdatabase/migrations/2026_05_05_000001_create_organizations_tables.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.phpdatabase/migrations/2026_05_05_000003_finalize_organizations_schema.phpdatabase/seeders/DatabaseSeeder.phpdocs/docs/user-guide/organizations.mddocs/docs/user-guide/permissions.mdresources/views/layouts/app.blade.phpresources/views/livewire/api-token/index.blade.phpresources/views/livewire/configuration/_tabs.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/organization-switcher.blade.phpresources/views/livewire/user/create.blade.phpresources/views/livewire/user/edit.blade.phpresources/views/livewire/user/index.blade.phproutes/web.phptests/Feature/Auth/OAuthTest.phptests/Feature/Auth/RegistrationTest.phptests/Feature/Dashboard/SnapshotsCardTest.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/DatabaseServer/EditTest.phptests/Feature/DatabaseServer/SshTunnelTest.phptests/Feature/DemoModeTest.phptests/Feature/Mcp/McpServerTest.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phptests/Feature/Services/DemoBackupServiceTest.phptests/Feature/User/CreateTest.phptests/Feature/User/EditTest.phptests/Feature/User/IndexTest.phptests/Feature/Volume/VolumeTest.phptests/Pest.phptests/Support/IntegrationTestHelpers.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (6)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
app/Policies/OrganizationPolicy.phpapp/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/StorageDistributionChart.phptests/Feature/Dashboard/SnapshotsCardTest.phpapp/Livewire/Forms/AgentForm.phpresources/views/livewire/configuration/_tabs.blade.phpconfig/oauth.phptests/Feature/Mcp/McpServerTest.phpresources/views/livewire/organization-switcher.blade.phpdatabase/factories/DatabaseServerSshConfigFactory.phpresources/views/layouts/app.blade.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phpapp/Livewire/Dashboard/LatestJobs.phpdatabase/factories/VolumeFactory.phproutes/web.phpdatabase/migrations/2026_05_05_000003_finalize_organizations_schema.phpapp/Actions/Fortify/CreateNewUser.phpapp/Models/BackupJob.phpapp/Livewire/Dashboard/JobStatusGrid.phpdatabase/factories/DatabaseServerFactory.phpdatabase/factories/AgentFactory.phpapp/Http/Controllers/Api/V1/DatabaseServerController.phpapp/Models/DatabaseServerSshConfig.phpapp/Livewire/Dashboard/StorageCard.phptests/Feature/DatabaseServer/EditTest.phpdatabase/factories/OrganizationFactory.phpresources/views/livewire/configuration/organization.blade.phpapp/Models/Agent.phpapp/Livewire/Forms/DatabaseServerForm.phpdatabase/migrations/2026_05_05_000001_create_organizations_tables.phpapp/Livewire/Dashboard/SuccessRateCard.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phpapp/Models/Snapshot.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Models/Volume.phptests/Feature/Services/DemoBackupServiceTest.phpapp/Http/Controllers/Api/V1/VolumeController.phpresources/views/livewire/api-token/index.blade.phpapp/Models/Organization.phptests/Pest.phpapp/Livewire/User/Edit.phpbootstrap/app.phpapp/Queries/SnapshotQuery.phpapp/Livewire/Dashboard/SnapshotsCard.phptests/Feature/User/IndexTest.phpapp/Models/DatabaseServer.phpapp/Livewire/Configuration/Organization.phpapp/Services/DemoBackupService.phptests/Support/IntegrationTestHelpers.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.phpresources/views/livewire/user/index.blade.phpapp/Policies/UserPolicy.phpapp/Services/CurrentOrganization.phpdatabase/seeders/DatabaseSeeder.phpapp/Livewire/User/Index.phpapp/Providers/AppServiceProvider.phptests/Feature/DatabaseServer/CreateTest.phpapp/Models/Scopes/OrganizationScope.phpapp/Livewire/Forms/VolumeForm.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phpapp/Http/Middleware/DemoModeMiddleware.phpapp/Livewire/User/Create.phpapp/Livewire/Forms/UserForm.phpdatabase/factories/UserFactory.phpapp/Livewire/OrganizationSwitcher.phpapp/Models/User.phpapp/Services/OAuthService.phpresources/views/livewire/user/edit.blade.phptests/Feature/Volume/VolumeTest.phpapp/Queries/BackupJobQuery.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.phpresources/views/livewire/user/create.blade.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
app/Policies/OrganizationPolicy.phpapp/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/StorageDistributionChart.phptests/Feature/Dashboard/SnapshotsCardTest.phpapp/Livewire/Forms/AgentForm.phpresources/views/livewire/configuration/_tabs.blade.phpconfig/oauth.phptests/Feature/Mcp/McpServerTest.phpresources/views/livewire/organization-switcher.blade.phpdatabase/factories/DatabaseServerSshConfigFactory.phpresources/views/layouts/app.blade.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phpapp/Livewire/Dashboard/LatestJobs.phpdatabase/factories/VolumeFactory.phproutes/web.phpdatabase/migrations/2026_05_05_000003_finalize_organizations_schema.phpapp/Actions/Fortify/CreateNewUser.phpapp/Models/BackupJob.phpapp/Livewire/Dashboard/JobStatusGrid.phpdatabase/factories/DatabaseServerFactory.phpdatabase/factories/AgentFactory.phpapp/Http/Controllers/Api/V1/DatabaseServerController.phpapp/Models/DatabaseServerSshConfig.phpapp/Livewire/Dashboard/StorageCard.phptests/Feature/DatabaseServer/EditTest.phpdatabase/factories/OrganizationFactory.phpresources/views/livewire/configuration/organization.blade.phpapp/Models/Agent.phpapp/Livewire/Forms/DatabaseServerForm.phpdatabase/migrations/2026_05_05_000001_create_organizations_tables.phpapp/Livewire/Dashboard/SuccessRateCard.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phpapp/Models/Snapshot.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Models/Volume.phptests/Feature/Services/DemoBackupServiceTest.phpapp/Http/Controllers/Api/V1/VolumeController.phpresources/views/livewire/api-token/index.blade.phpapp/Models/Organization.phptests/Pest.phpapp/Livewire/User/Edit.phpbootstrap/app.phpapp/Queries/SnapshotQuery.phpapp/Livewire/Dashboard/SnapshotsCard.phptests/Feature/User/IndexTest.phpapp/Models/DatabaseServer.phpapp/Livewire/Configuration/Organization.phpapp/Services/DemoBackupService.phptests/Support/IntegrationTestHelpers.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.phpresources/views/livewire/user/index.blade.phpapp/Policies/UserPolicy.phpapp/Services/CurrentOrganization.phpdatabase/seeders/DatabaseSeeder.phpapp/Livewire/User/Index.phpapp/Providers/AppServiceProvider.phptests/Feature/DatabaseServer/CreateTest.phpapp/Models/Scopes/OrganizationScope.phpapp/Livewire/Forms/VolumeForm.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phpapp/Http/Middleware/DemoModeMiddleware.phpapp/Livewire/User/Create.phpapp/Livewire/Forms/UserForm.phpdatabase/factories/UserFactory.phpapp/Livewire/OrganizationSwitcher.phpapp/Models/User.phpapp/Services/OAuthService.phpresources/views/livewire/user/edit.blade.phptests/Feature/Volume/VolumeTest.phpapp/Queries/BackupJobQuery.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.phpresources/views/livewire/user/create.blade.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/StorageDistributionChart.phpapp/Livewire/Forms/AgentForm.phpapp/Livewire/Dashboard/LatestJobs.phpapp/Livewire/Dashboard/JobStatusGrid.phpapp/Livewire/Dashboard/StorageCard.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Livewire/Dashboard/SuccessRateCard.phpapp/Livewire/User/Edit.phpapp/Livewire/Dashboard/SnapshotsCard.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/User/Index.phpapp/Livewire/Forms/VolumeForm.phpapp/Livewire/User/Create.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/OrganizationSwitcher.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model.
Use faker methods such as$this->faker->word()orfake()->randomDigit()when generating test data. Follow existing conventions on whether to use$this->fakerorfake().
Create Pest tests usingphp artisan make:test --pest {name}without including the test suite directory. Usephp artisan make:test --pest SomeFeatureTestinstead ofphp artisan make:test --pest Feature/SomeFeatureTest.
Files:
tests/Feature/Dashboard/SnapshotsCardTest.phptests/Feature/Mcp/McpServerTest.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phptests/Feature/DatabaseServer/EditTest.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phptests/Feature/Services/DemoBackupServiceTest.phptests/Pest.phptests/Feature/User/IndexTest.phptests/Support/IntegrationTestHelpers.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phptests/Feature/Volume/VolumeTest.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
All Livewire component blade files should contain only view markup; all PHP logic must be in the component class.
Files:
resources/views/livewire/configuration/_tabs.blade.phpresources/views/livewire/organization-switcher.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/api-token/index.blade.phpresources/views/livewire/user/index.blade.phpresources/views/livewire/user/edit.blade.phpresources/views/livewire/user/create.blade.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components in Mary UI should be prefixed withx-(e.g.,<x-button>,<x-input>,<x-card>) and use Heroicons for icons (e.g.,icon="o-user"for outline,icon="s-user"for solid).
In Mary UI select components, use the:optionsprop with array format[['id' => 'value', 'name' => 'Label']].
In Mary UI alerts, useclass="alert-success",class="alert-error", etc. (notvariantprop).
Use Alpine.js for client-side interactions in Livewire instead of JavaScript frameworks.
In Blade templates, use:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters.
Files:
resources/views/livewire/configuration/_tabs.blade.phpresources/views/livewire/organization-switcher.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/api-token/index.blade.phpresources/views/livewire/user/index.blade.phpresources/views/livewire/user/edit.blade.phpresources/views/livewire/user/create.blade.php
🧠 Learnings (10)
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Policies/OrganizationPolicy.phpapp/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/StorageDistributionChart.phptests/Feature/Dashboard/SnapshotsCardTest.phpapp/Livewire/Forms/AgentForm.phpresources/views/livewire/configuration/_tabs.blade.phpconfig/oauth.phptests/Feature/Mcp/McpServerTest.phpresources/views/livewire/organization-switcher.blade.phpdatabase/factories/DatabaseServerSshConfigFactory.phpresources/views/layouts/app.blade.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phpapp/Livewire/Dashboard/LatestJobs.phpdatabase/factories/VolumeFactory.phproutes/web.phpdatabase/migrations/2026_05_05_000003_finalize_organizations_schema.phpapp/Actions/Fortify/CreateNewUser.phpapp/Models/BackupJob.phpapp/Livewire/Dashboard/JobStatusGrid.phpdatabase/factories/DatabaseServerFactory.phpdatabase/factories/AgentFactory.phpapp/Http/Controllers/Api/V1/DatabaseServerController.phpapp/Models/DatabaseServerSshConfig.phpapp/Livewire/Dashboard/StorageCard.phptests/Feature/DatabaseServer/EditTest.phpdatabase/factories/OrganizationFactory.phpresources/views/livewire/configuration/organization.blade.phpapp/Models/Agent.phpapp/Livewire/Forms/DatabaseServerForm.phpdatabase/migrations/2026_05_05_000001_create_organizations_tables.phpapp/Livewire/Dashboard/SuccessRateCard.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phpapp/Models/Snapshot.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Models/Volume.phptests/Feature/Services/DemoBackupServiceTest.phpapp/Http/Controllers/Api/V1/VolumeController.phpresources/views/livewire/api-token/index.blade.phpapp/Models/Organization.phptests/Pest.phpapp/Livewire/User/Edit.phpbootstrap/app.phpapp/Queries/SnapshotQuery.phpapp/Livewire/Dashboard/SnapshotsCard.phptests/Feature/User/IndexTest.phpapp/Models/DatabaseServer.phpapp/Livewire/Configuration/Organization.phpapp/Services/DemoBackupService.phptests/Support/IntegrationTestHelpers.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.phpresources/views/livewire/user/index.blade.phpapp/Policies/UserPolicy.phpapp/Services/CurrentOrganization.phpdatabase/seeders/DatabaseSeeder.phpapp/Livewire/User/Index.phpapp/Providers/AppServiceProvider.phptests/Feature/DatabaseServer/CreateTest.phpapp/Models/Scopes/OrganizationScope.phpapp/Livewire/Forms/VolumeForm.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phpapp/Http/Middleware/DemoModeMiddleware.phpapp/Livewire/User/Create.phpapp/Livewire/Forms/UserForm.phpdatabase/factories/UserFactory.phpapp/Livewire/OrganizationSwitcher.phpapp/Models/User.phpapp/Services/OAuthService.phpresources/views/livewire/user/edit.blade.phptests/Feature/Volume/VolumeTest.phpapp/Queries/BackupJobQuery.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.phpresources/views/livewire/user/create.blade.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Dashboard/JobsActivityChart.phpapp/Livewire/Dashboard/StorageDistributionChart.phpapp/Livewire/Forms/AgentForm.phpapp/Livewire/Dashboard/LatestJobs.phpapp/Livewire/Dashboard/JobStatusGrid.phpapp/Livewire/Dashboard/StorageCard.phpapp/Livewire/Forms/DatabaseServerForm.phpapp/Livewire/Dashboard/SuccessRateCard.phpapp/Livewire/User/Edit.phpapp/Livewire/Dashboard/SnapshotsCard.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/User/Index.phpapp/Livewire/Forms/VolumeForm.phpapp/Livewire/User/Create.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/OrganizationSwitcher.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Dashboard/SnapshotsCardTest.phptests/Feature/Mcp/McpServerTest.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phptests/Feature/DatabaseServer/EditTest.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phptests/Feature/Services/DemoBackupServiceTest.phptests/Pest.phptests/Feature/User/IndexTest.phptests/Support/IntegrationTestHelpers.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phptests/Feature/Volume/VolumeTest.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.php
📚 Learning: 2026-04-09T13:59:25.873Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: tests/Feature/ConfigurationTest.php:165-176
Timestamp: 2026-04-09T13:59:25.873Z
Learning: In this repository, `Notification::fake()` is already performed globally in the base test class (`tests/TestCase.php`) within `setUp()` before each test. When reviewing individual test files under `tests/`, do not flag missing `Notification::fake()` calls, since they are handled by the shared base test setup.
Applied to files:
tests/Feature/Dashboard/SnapshotsCardTest.phptests/Feature/Mcp/McpServerTest.phptests/Feature/Services/Backup/LocalVolumeStorageTest.phptests/Feature/DatabaseServer/EditTest.phptests/Feature/DemoModeTest.phptests/Feature/DatabaseServer/SshTunnelTest.phptests/Feature/Services/DemoBackupServiceTest.phptests/Pest.phptests/Feature/User/IndexTest.phptests/Support/IntegrationTestHelpers.phptests/Feature/DatabaseServer/CreateTest.phptests/Feature/User/CreateTest.phptests/Feature/Auth/OAuthTest.phptests/Feature/Volume/VolumeTest.phptests/Feature/Auth/RegistrationTest.phptests/Feature/User/EditTest.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/configuration/_tabs.blade.phpresources/views/livewire/organization-switcher.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/api-token/index.blade.phpresources/views/livewire/user/index.blade.phpresources/views/livewire/user/edit.blade.phpresources/views/livewire/user/create.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/configuration/_tabs.blade.phpresources/views/livewire/organization-switcher.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/api-token/index.blade.phpresources/views/livewire/user/index.blade.phpresources/views/livewire/user/edit.blade.phpresources/views/livewire/user/create.blade.php
📚 Learning: 2026-02-04T12:26:27.374Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 72
File: routes/web.php:66-81
Timestamp: 2026-02-04T12:26:27.374Z
Learning: In Laravel Livewire v4, prefer routing full-page Livewire components using Route::livewire('/path', ComponentClass::class) for all full-page components (whether class-based or view-based). While Route::get('/path', ComponentClass::class) remains technically functional for backward compatibility, it is not recommended in v4. Apply this guideline in all route files under routes (e.g., routes/web.php, routes/api.php) to ensure consistent, future-proof routing for Livewire components.
Applied to files:
routes/web.php
📚 Learning: 2026-04-10T08:46:55.182Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: database/migrations/2026_04_09_000001_add_notification_channels.php:143-156
Timestamp: 2026-04-10T08:46:55.182Z
Learning: In David-Crty/databasement, treat Laravel migrations as forward-only for production. For migrations in database/migrations/*.php, it is acceptable to delete or transform legacy data in the migration’s up() (e.g., removing app_configs rows) without restoring it in down(), even if down() cannot fully reverse the state. During code review, do not flag missing rollback restoration of migrated/deleted data as a production risk for this repo’s migrations.
Applied to files:
database/migrations/2026_05_05_000003_finalize_organizations_schema.phpdatabase/migrations/2026_05_05_000001_create_organizations_tables.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.php
📚 Learning: 2026-03-23T11:45:15.846Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 178
File: app/Http/Controllers/Api/V1/VolumeController.php:116-120
Timestamp: 2026-03-23T11:45:15.846Z
Learning: For the API v1 `testConnection` endpoints in this repo (e.g., `VolumeController::testConnection`, `DatabaseServerController::testConnection`), keep using `$this->authorize('view', $model)` for authorization. This is an intentional maintainer design choice; do not flag it as a bug or as insufficiently strict authorization in future reviews for these `testConnection` actions.
Applied to files:
app/Http/Controllers/Api/V1/DatabaseServerController.phpapp/Http/Controllers/Api/V1/VolumeController.php
📚 Learning: 2026-03-28T22:33:16.333Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 195
File: app/Notifications/CLAUDE.md:7-12
Timestamp: 2026-03-28T22:33:16.333Z
Learning: In this repository (David-Crty/databasement), do not flag fenced Markdown code blocks that omit a language identifier (i.e., MD040: missing language after ```). The project treats unlabeled fenced code blocks as plain text by default, so missing language tags are acceptable for Markdown files.
Applied to files:
docs/docs/user-guide/permissions.mddocs/docs/user-guide/organizations.md
🪛 markdownlint-cli2 (0.22.1)
docs/docs/user-guide/organizations.md
[warning] 65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
app/Services/CurrentOrganization.php (1)
44-55: ⚡ Quick winReuse
User::roleIn()for the current-org lookup.This method bypasses the eager-loaded/cache-aware path added in
User::roleIn(), souserRole()still does a fresh pivot query even whenorganizationsis already loaded. ReusingroleIn($this->organization)keeps a single source of truth and preserves the N+1 fix elsewhere in the PR.Proposed change
- $pivot = $user->organizations()->wherePivot('organization_id', $this->organization->id)->first(); - - return $pivot?->pivot?->role; // `@phpstan-ignore` property.notFound + return $user->roleIn($this->organization);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/CurrentOrganization.php` around lines 44 - 55, Replace the manual pivot lookup in userRole() with the existing User::roleIn() helper to preserve eager-loaded/cache-aware behavior: obtain the current user (as you already do), return null if missing, then call $user->roleIn($this->organization) and return its result instead of performing the organizations()->wherePivot(...) pivot query; remove the manual pivot access so userRole() delegates to User::roleIn() as the single source of truth.app/Livewire/Configuration/Organization.php (1)
44-48: ⚡ Quick winOnly bypass
OrganizationScopein these count subqueries.
withoutGlobalScopes()fixes the org-count bug, but it also drops any other global scope those models may gain later. These counts only need to ignore tenant scoping.Proposed change
+use App\Models\Scopes\OrganizationScope; ... - 'databaseServers' => fn ($q) => $q->withoutGlobalScopes(), - 'volumes' => fn ($q) => $q->withoutGlobalScopes(), - 'agents' => fn ($q) => $q->withoutGlobalScopes(), + 'databaseServers' => fn ($q) => $q->withoutGlobalScope(OrganizationScope::class), + 'volumes' => fn ($q) => $q->withoutGlobalScope(OrganizationScope::class), + 'agents' => fn ($q) => $q->withoutGlobalScope(OrganizationScope::class),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Configuration/Organization.php` around lines 44 - 48, The count subqueries on OrganizationModel are calling withoutGlobalScopes(), which removes all global scopes (risking future scope loss); change each subquery for 'databaseServers', 'volumes', and 'agents' to only remove the tenant OrganizationScope (e.g., use withoutGlobalScope(OrganizationScope::class)) so the users/databaseServers/volumes/agents counts bypass only tenant scoping while preserving other global scopes; locate these calls on OrganizationModel::withCount([...]) and replace withoutGlobalScopes() with withoutGlobalScope(OrganizationScope::class).app/Livewire/Forms/UserForm.php (1)
21-22: ⚡ Quick winUse the role constants in the validators too.
roleOptions()already derives the allowed values fromUser::ROLE_*, but the validation rules still hardcode'viewer,member,admin'. That gives this form two separate definitions of the same contract.Proposed change
- #[Validate('required|in:viewer,member,admin')] + #[Validate('required|in:'.User::ROLE_VIEWER.','.User::ROLE_MEMBER.','.User::ROLE_ADMIN)] public string $role = User::ROLE_MEMBER; ... - 'role' => 'required|in:viewer,member,admin', + 'role' => 'required|in:'.User::ROLE_VIEWER.','.User::ROLE_MEMBER.','.User::ROLE_ADMIN, ... - 'role' => 'required|in:viewer,member,admin', + 'role' => 'required|in:'.User::ROLE_VIEWER.','.User::ROLE_MEMBER.','.User::ROLE_ADMIN,Also applies to: 40-44, 63-67
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Forms/UserForm.php` around lines 21 - 22, The Validate attribute on the $role property hardcodes allowed roles; remove that hardcoded attribute and move the rule into a dynamic rules() method (or update the existing rules provider) so it builds the in: list from the source of truth (User::roleOptions() or the User::ROLE_* constants) — e.g. return ['role' => 'required|in:' . implode(',', array_values(User::roleOptions()))]; apply the same change to the other occurrences noted (the other Validate usages at the referenced blocks) so validation always derives allowed values from User::ROLE_* rather than a hardcoded string.app/Livewire/User/Index.php (1)
142-156: ⚡ Quick winConsolidate redundant org filters and drop
whereRaw.When
roleFilteris set, the query emits two EXISTS subqueries onorganization_userfor the sameorganization_id— one fromwhereRelation(line 144) and one fromwhereHas(line 155). They can be folded into a singlewhereHasthat also covers the role predicate, removing the duplicate EXISTS and thewhereRaw.♻️ Proposed refactor
$currentOrg = app(CurrentOrganization::class); - $query = User::query(); - - $query->whereRelation('organizations', 'organization_id', $currentOrg->id()); - - $users = $query + $users = User::query() ->with('organizations') + ->whereHas('organizations', function ($q) use ($currentOrg) { + $q->where('organization_id', $currentOrg->id()); + if ($this->roleFilter !== '') { + $q->where('role', $this->roleFilter); + } + }) ->when($this->search, function ($query) { $query->where(function ($q) { $q->where('name', 'like', '%'.$this->search.'%') ->orWhere('email', 'like', '%'.$this->search.'%'); }); }) - ->when($this->roleFilter !== '', function ($query) use ($currentOrg) { - $query->whereHas('organizations', fn ($q) => $q->whereRaw('organization_id = ? and role = ?', [$currentOrg->id(), $this->roleFilter])); - }) ->when($this->statusFilter !== '', function ($query) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/User/Index.php` around lines 142 - 156, Remove the initial whereRelation call and consolidate the organization filter into the existing whereHas on the organizations relation: use User::query()->when($this->search, ...)->whereHas('organizations', function($q) use ($currentOrg) { $q->where('organization_id', $currentOrg->id()); if ($this->roleFilter !== '') { $q->where('role', $this->roleFilter); } }); this removes the duplicate EXISTS and replaces whereRaw with explicit where(...) calls on the organizations relation (keep the ->with('organizations') and existing search logic intact).tests/Feature/User/CreateTest.php (1)
59-79: 💤 Low valueNit: import
Organizationinstead of repeating the FQN.
\App\Models\Organizationis referenced three times (lines 59, 67, 78). Ausestatement at the top would match the convention used intests/Feature/Configuration/OrganizationTest.php(which aliases it asOrganizationModel).♻️ Proposed cleanup
use App\Livewire\User\Create; +use App\Models\Organization; use App\Models\User; use Livewire\Livewire; @@ - expect($user->roleIn(\App\Models\Organization::main()))->toBe('member'); + expect($user->roleIn(Organization::main()))->toBe('member'); @@ - $otherOrg = \App\Models\Organization::factory()->create(); + $otherOrg = Organization::factory()->create(); @@ - expect($existingUser->roleIn(\App\Models\Organization::main()))->toBe('viewer'); + expect($existingUser->roleIn(Organization::main()))->toBe('viewer');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/User/CreateTest.php` around lines 59 - 79, Import the Organization model at the top and replace the three FQN usages to match the project convention; add a use statement for App\Models\Organization (or alias it as OrganizationModel to follow OrganizationTest) and change the three occurrences of \App\Models\Organization::main() in the CreateTest Livewire test to Organization::main() (or OrganizationModel::main() if aliased) so the test uses the imported symbol instead of the fully-qualified names.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Http/Middleware/SetCurrentOrganization.php`:
- Around line 45-51: The code only treats non-empty org query/header values as
explicit; change the $hasExplicitOrg check in SetCurrentOrganization to detect
presence even if blank by using the underlying parameter bags: use
$request->query->has('org') || $request->headers->has('X-Organization-Id') so an
explicit but empty selector (e.g. ?org= or empty header) will be considered
explicit and thus trigger the existing 403 logic after resolveApiOrgId and
currentOrganization->resolveForUser.
In `@app/Livewire/User/Create.php`:
- Around line 55-63: The attach call can race and violate the pivot unique
constraint; make the insert idempotent by replacing the
organizations()->attach(...) with an idempotent write (e.g.
organizations()->syncWithoutDetaching([$currentOrg->id() => ['role' =>
$this->existingUserRole]])) or wrap the organizations()->attach(...) in a
try/catch for QueryException and call $this->addError('existingUserId', __('This
user is already a member of this organization.')) when a duplicate-key error is
detected; locate this logic around User::findOrFail($this->existingUserId),
belongsToOrganization(...), organizations()->attach(...) and update accordingly.
In `@app/Models/User.php`:
- Around line 188-216: roleIn() memoizes per-organization roles in $cachedRoles
but that cache is only cleared by refresh(), so pivots updated via attach() or
updateExistingPivot() can leave stale values; add a small invalidation helper on
the User model (e.g., clearCachedRoleForOrg(int|string $orgId) or
clearCachedRoles(?int $orgId) that unsets $this->cachedRoles[$orgId] (or clears
the whole array if null) and invoke this helper immediately after any pivot
write in your write paths (calls that use attach() or updateExistingPivot()),
for example from UserForm::setUser() and UserForm::update() after they modify
the organization pivot so roleIn()/isAdmin() will reflect the new pivot without
needing refresh().
In `@app/Policies/UserPolicy.php`:
- Around line 86-89: The copyInvitationLink policy currently restricts access to
super admins only; update the UserPolicy::copyInvitationLink method to mirror
the logic used in update() by allowing either a super admin OR an org admin of
the same organization to copy the invitation link, while still requiring the
model to have a non-null invitation_token; use existing helpers like
isSuperAdmin() and isOrgAdmin() and compare the current user's org id to
$model->org_id to implement this relaxation.
In `@tests/Feature/OrganizationSwitcherTest.php`:
- Around line 19-27: The test calls OrganizationSwitcher::switchOrg but only
asserts no redirect; capture the component's initial currentOrgId (or the user's
first organization id) before invoking switchOrg and after the call assert that
the component's "currentOrgId" remains equal to that initial value (use
OrganizationSwitcher, the switchOrg method and the currentOrgId property) so the
negative path verifies the org did not change.
---
Nitpick comments:
In `@app/Livewire/Configuration/Organization.php`:
- Around line 44-48: The count subqueries on OrganizationModel are calling
withoutGlobalScopes(), which removes all global scopes (risking future scope
loss); change each subquery for 'databaseServers', 'volumes', and 'agents' to
only remove the tenant OrganizationScope (e.g., use
withoutGlobalScope(OrganizationScope::class)) so the
users/databaseServers/volumes/agents counts bypass only tenant scoping while
preserving other global scopes; locate these calls on
OrganizationModel::withCount([...]) and replace withoutGlobalScopes() with
withoutGlobalScope(OrganizationScope::class).
In `@app/Livewire/Forms/UserForm.php`:
- Around line 21-22: The Validate attribute on the $role property hardcodes
allowed roles; remove that hardcoded attribute and move the rule into a dynamic
rules() method (or update the existing rules provider) so it builds the in: list
from the source of truth (User::roleOptions() or the User::ROLE_* constants) —
e.g. return ['role' => 'required|in:' . implode(',',
array_values(User::roleOptions()))]; apply the same change to the other
occurrences noted (the other Validate usages at the referenced blocks) so
validation always derives allowed values from User::ROLE_* rather than a
hardcoded string.
In `@app/Livewire/User/Index.php`:
- Around line 142-156: Remove the initial whereRelation call and consolidate the
organization filter into the existing whereHas on the organizations relation:
use User::query()->when($this->search, ...)->whereHas('organizations',
function($q) use ($currentOrg) { $q->where('organization_id',
$currentOrg->id()); if ($this->roleFilter !== '') { $q->where('role',
$this->roleFilter); } }); this removes the duplicate EXISTS and replaces
whereRaw with explicit where(...) calls on the organizations relation (keep the
->with('organizations') and existing search logic intact).
In `@app/Services/CurrentOrganization.php`:
- Around line 44-55: Replace the manual pivot lookup in userRole() with the
existing User::roleIn() helper to preserve eager-loaded/cache-aware behavior:
obtain the current user (as you already do), return null if missing, then call
$user->roleIn($this->organization) and return its result instead of performing
the organizations()->wherePivot(...) pivot query; remove the manual pivot access
so userRole() delegates to User::roleIn() as the single source of truth.
In `@tests/Feature/User/CreateTest.php`:
- Around line 59-79: Import the Organization model at the top and replace the
three FQN usages to match the project convention; add a use statement for
App\Models\Organization (or alias it as OrganizationModel to follow
OrganizationTest) and change the three occurrences of
\App\Models\Organization::main() in the CreateTest Livewire test to
Organization::main() (or OrganizationModel::main() if aliased) so the test uses
the imported symbol instead of the fully-qualified names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 442da78a-9ab4-4658-8a59-437c107f2bbc
📒 Files selected for processing (14)
app/Http/Middleware/SetCurrentOrganization.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/User/Create.phpapp/Livewire/User/Index.phpapp/Models/BackupJob.phpapp/Models/Organization.phpapp/Models/Snapshot.phpapp/Models/User.phpapp/Policies/UserPolicy.phpapp/Services/CurrentOrganization.phptests/Feature/Configuration/OrganizationTest.phptests/Feature/OrganizationSwitcherTest.phptests/Feature/User/CreateTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Models/Organization.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
app/Models/BackupJob.phptests/Feature/OrganizationSwitcherTest.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Services/CurrentOrganization.phpapp/Livewire/User/Index.phpapp/Policies/UserPolicy.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.phpapp/Models/Snapshot.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Models/User.phpapp/Livewire/User/Create.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
app/Models/BackupJob.phptests/Feature/OrganizationSwitcherTest.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Services/CurrentOrganization.phpapp/Livewire/User/Index.phpapp/Policies/UserPolicy.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.phpapp/Models/Snapshot.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Models/User.phpapp/Livewire/User/Create.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model.
Use faker methods such as$this->faker->word()orfake()->randomDigit()when generating test data. Follow existing conventions on whether to use$this->fakerorfake().
Create Pest tests usingphp artisan make:test --pest {name}without including the test suite directory. Usephp artisan make:test --pest SomeFeatureTestinstead ofphp artisan make:test --pest Feature/SomeFeatureTest.
Files:
tests/Feature/OrganizationSwitcherTest.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/User/Index.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/User/Create.php
🧠 Learnings (5)
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Models/BackupJob.phptests/Feature/OrganizationSwitcherTest.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Services/CurrentOrganization.phpapp/Livewire/User/Index.phpapp/Policies/UserPolicy.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.phpapp/Models/Snapshot.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Models/User.phpapp/Livewire/User/Create.php
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/OrganizationSwitcherTest.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.php
📚 Learning: 2026-04-09T13:59:25.873Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: tests/Feature/ConfigurationTest.php:165-176
Timestamp: 2026-04-09T13:59:25.873Z
Learning: In this repository, `Notification::fake()` is already performed globally in the base test class (`tests/TestCase.php`) within `setUp()` before each test. When reviewing individual test files under `tests/`, do not flag missing `Notification::fake()` calls, since they are handled by the shared base test setup.
Applied to files:
tests/Feature/OrganizationSwitcherTest.phptests/Feature/User/CreateTest.phptests/Feature/Configuration/OrganizationTest.php
📚 Learning: 2026-05-05T23:16:24.464Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Services/CurrentOrganization.php:28-39
Timestamp: 2026-05-05T23:16:24.464Z
Learning: Enforce non-nullable return types for id(): string and model(): Organization in PHP service classes under app/Services. Ensure SetCurrentOrganization middleware always resolves an Organization in the web context before these methods are reached; do not reintroduce isResolved guards in callers. A null dereference due to a broken middleware chain is the intended failure signal, not silent null propagation. Document the contract in class docblocks and consider tests to verify the middleware guarantees.
Applied to files:
app/Services/CurrentOrganization.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/User/Index.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/Forms/UserForm.phpapp/Livewire/User/Create.php
🔇 Additional comments (2)
app/Policies/UserPolicy.php (1)
45-56: Super-admin guard onupdate()looks correct.Org admins can no longer edit super-admin users that happen to be members of their org, and the existing self/membership conditions are preserved. Matches the addressed feedback from prior reviews.
tests/Feature/Configuration/OrganizationTest.php (1)
12-102: LGTM — solid coverage of Organization management flows.Access control, validation, rename main/non-main, delete main/non-main/with-resources, and the empty-name validation paths are all exercised. The resource-scoping check at line 96 also confirms
Organization::hasResources()'s scope-bypass behavior referenced in the PR description.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
resources/views/layouts/app.blade.php (1)
62-62:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
"Super Admin"badge value is not wrapped in__()translation helper.The static string
value="Super Admin"bypasses the translation pipeline. Consistent with how all other user-facing strings are handled across the file, this should use the dynamic binding syntax.🔧 Proposed fix
- <x-badge value="Super Admin" icon="o-star" class="badge-warning whitespace-nowrap" /> + <x-badge :value="__('Super Admin')" icon="o-star" class="badge-warning whitespace-nowrap" />As per coding guidelines: "Translations should use
__('...')helper function" and "use:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/layouts/app.blade.php` at line 62, The "Super Admin" badge is using a static value attribute and not going through the translation helper; update the badge component usage (the element using value="Super Admin", e.g., the <x-badge> or similar component) to use the translation helper and dynamic binding by replacing value="Super Admin" with :value="__('Super Admin')" so the string is translatable and uses the :attr binding pattern consistent with other strings in this file.
🧹 Nitpick comments (8)
resources/views/livewire/user/create.blade.php (1)
60-74: 💤 Low valueDuplicate role label rendered on the group.
Both blocks render a manual
<label>("Role in current organization") immediately followed by<x-radio-card-group ... :label="__('Role')">, which causes the component to render its own internal "Role" label as well. Drop one of them so only a single, clear label is shown.♻️ Proposed fix
- <div> - <label class="label label-text font-semibold mb-2">{{ __('Role in current organization') }}</label> - <x-radio-card-group class="grid-cols-1 sm:grid-cols-3" :label="__('Role')"> + <div> + <x-radio-card-group class="grid-cols-1 sm:grid-cols-3" :label="__('Role in current organization')">Also applies to: 93-107
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/user/create.blade.php` around lines 60 - 74, The UI renders two labels for the role field because a manual <label> element is placed immediately before the <x-radio-card-group> which also outputs its own label via the :label prop; remove one to avoid duplicate text—either delete the manual <label class="label label-text...">{{ __('Role in current organization') }}</label> or remove the :label="__('Role')" prop on the <x-radio-card-group> component (same change must be applied in the duplicate block near the bottom of the file that mirrors this role field).app/Livewire/User/Index.php (1)
147-161: 💤 Low valueConsolidate the org + role filter into one
whereHasand dropwhereRaw.As written, you produce two independent
EXISTSsubqueries againstorganization_user(one fromwhereRelation, another fromwhereHaswhenroleFilteris set), and the role-filter branch uses raw SQL where it isn't needed. Both filters target the same pivot relationship and can be expressed once with a parameterisedwhere:♻️ Proposed fix
- $query = User::query(); - - $query->whereRelation('organizations', 'organization_id', $currentOrg->id()); - - $users = $query - ->with('organizations') + $orgId = $currentOrg->id(); + + $users = User::query() + ->with('organizations') + ->whereHas('organizations', function ($q) use ($orgId) { + $q->where('organization_id', $orgId); + }) ->when($this->search, function ($query) { $query->where(function ($q) { $q->where('name', 'like', '%'.$this->search.'%') ->orWhere('email', 'like', '%'.$this->search.'%'); }); }) - ->when($this->roleFilter !== '', function ($query) use ($currentOrg) { - $query->whereHas('organizations', fn ($q) => $q->whereRaw('organization_id = ? and role = ?', [$currentOrg->id(), $this->roleFilter])); - }) + ->when($this->roleFilter !== '', function ($query) use ($orgId) { + $query->whereHas('organizations', fn ($q) => $q + ->where('organization_id', $orgId) + ->where('role', $this->roleFilter)); + })Note: the outer
whereHasand the role-scopedwhereHasstill produce twoEXISTSsubqueries. If you want a single one, you can keep only the role-scoped variant and make it the always-applied org filter, switching towhere('role', $this->roleFilter)only when set.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/User/Index.php` around lines 147 - 161, Replace the separate whereRelation + conditional whereHas with a single whereHas on the 'organizations' relation: remove the call to whereRelation('organizations', ...) and instead call whereHas('organizations', function($q) use ($currentOrg) { $q->where('organization_id', $currentOrg->id()); if ($this->roleFilter !== '') { $q->where('role', $this->roleFilter); } }); — this drops the raw SQL, keeps the organization filter and only applies the role filter when set, preserving the existing with('organizations') and search logic.tests/Feature/User/CreateTest.php (1)
87-115: ⚡ Quick winTest relies on implicit factory side-effect (attachment to Main org).
Both tests assume
User::factory()->create()attaches the new user toOrganization::main()by default — line 92-94 deliberatelysyncs to a different org to remove that membership, while line 105-115 relies on it to assert "already a member". This implicit dependency on factory behaviour is fragile; if the default attachment ever changes, both tests will fail in non-obvious ways.Consider making the membership state explicit at the start of each test (e.g., explicitly attach/detach in arrange) to document the precondition.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/User/CreateTest.php` around lines 87 - 115, The tests for the Create Livewire component rely on User::factory()->create() implicitly attaching users to Organization::main(); make the membership explicit: in the "admin can add existing user" test ensure the created $existingUser is detached from Organization::main() (e.g., call $existingUser->organizations()->detach(Organization::main()->id) or sync to the other org) before calling Livewire::test(Create::class), and in the "rejects adding user already in organization" test explicitly attach the $existingUser to Organization::main() with the expected role (e.g., $existingUser->organizations()->attach(Organization::main()->id, ['role' => 'member'])) so the preconditions for Create::addExisting and the roleIn(Organization::main()) assertions are obvious and stable.tests/Feature/User/EditTest.php (1)
51-57: 💤 Low valuePrefer factory states over
['role' => $role]for consistency.Same pattern as
IndexTest::non-admin cannot access user index: the dataset values'member'/'viewer'correspond to existing factory states used elsewhere in this PR. Switching toUser::factory()->{$state}()->create()ensures the per-organization pivot role is set in addition to the legacy column.As per coding guidelines: "Check if the factory has custom states before manually setting up the model."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/User/EditTest.php` around lines 51 - 57, The test currently creates non-admin users with User::factory()->create(['role' => $role]) which bypasses factory states and misses per-organization pivot role; update the test 'non-admin cannot edit users' to use the factory states instead (e.g. User::factory()->member()->create() and User::factory()->viewer()->create()) so the per-organization pivot role is set correctly; keep the rest of the test (the actingAs($user) call and get(route('users.edit', $target))->assertForbidden()) unchanged and ensure the ->with([...]) data provider values match the state method names.resources/views/livewire/configuration/organization.blade.php (1)
12-12: 💤 Low valueAdd
rel="noopener noreferrer"to external_blanklink.External link with
target="_blank"should setrel="noopener noreferrer"to prevent the opened tab from accessingwindow.openerand to avoid leaking the referrer.♻️ Proposed fix
- <a href="https://david-crty.github.io/databasement/docs/user-guide/organizations" target="_blank" class="link link-primary">{{ __('Learn more') }}</a> + <a href="https://david-crty.github.io/databasement/docs/user-guide/organizations" target="_blank" rel="noopener noreferrer" class="link link-primary">{{ __('Learn more') }}</a>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/configuration/organization.blade.php` at line 12, The external anchor tag rendering the "Learn more" link uses target="_blank" but is missing rel="noopener noreferrer"; update the <a> element in the organization Blade view (the anchor with class "link link-primary" and text "Learn more") to include rel="noopener noreferrer" so the new tab cannot access window.opener and the referrer is not leaked.app/Livewire/User/Create.php (1)
53-56: ⚡ Quick winKeep the existing-user rules on the component or form object.
addExisting()adds another inline rule array even though this component already usesUserFormfor the invite flow. Please moveexistingUserId/existingUserRoleto#[Validate]attributes or a form object so both modes follow the same Livewire pattern.As per coding guidelines: "In Livewire components, use
#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/User/Create.php` around lines 53 - 56, The inline validation for existingUserId and existingUserRole inside the Livewire component should be moved to the component's validation declarations instead of calling $this->validate() with an ad-hoc array in addExisting(); update the component to use #[Validate] attributes or include these fields on the existing UserForm so both invite and addExisting() flows reuse the same rules (reference existingUserId, existingUserRole, addExisting(), UserForm and #[Validate]) and remove the duplicate inline rule array so validation follows the Livewire form/attribute pattern.app/Livewire/Configuration/Organization.php (2)
76-76: ⚡ Quick winUse Livewire navigate redirects after these actions.
These plain redirects force a full page load. Prefer
$this->redirect(..., navigate: true)here so organization CRUD stays consistent with the app’s SPA-style navigation.↪️ Minimal change
- return $this->redirect(route('configuration.organizations')); + return $this->redirect(route('configuration.organizations'), navigate: true);As per coding guidelines: "In Livewire components, return
$this->redirect()withnavigate: truefor SPA-like navigation."Also applies to: 105-105, 129-129
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Configuration/Organization.php` at line 76, The redirect calls in the Livewire component Organization.php perform full page loads; replace each plain redirect call (the return $this->redirect(route('configuration.organizations')) instances at the three spots) with Livewire navigation-enabled redirects by calling $this->redirect(..., navigate: true) so the component uses SPA-style navigation; update the redirect invocations in the methods that currently return redirects (the Organization Livewire action methods around the redirect lines) accordingly.
65-67: ⚡ Quick winMove these organization-name rules into Livewire state.
Both actions define ad-hoc validation arrays inside the methods. Please keep
newOrgName/editOrgNameon#[Validate]attributes or a small form object so this component follows the repo’s Livewire validation pattern.As per coding guidelines: "In Livewire components, use
#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data."Also applies to: 96-98
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Configuration/Organization.php` around lines 65 - 67, The inline validation arrays for newOrgName (and similarly editOrgName) should be moved out of the method and defined as Livewire validation metadata — add #[Validate] attributes (or a small Form object) on the Organization component properties newOrgName and editOrgName with the rules 'required|string|max:255|unique:organizations,name' (and the corresponding edit rule), then remove the ad-hoc arrays from the methods (at lines calling $this->validate([...])) so the methods simply call $this->validate() to trigger the attribute-based validation; update any references in the create/edit handlers to rely on the validated properties.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Policies/UserPolicy.php`:
- Around line 74-75: The policy's check using $model->isSuperAdmin() and
User::where('super_admin', true)->count() is racy because it runs outside the
delete transaction; to fix, move the "last super admin" invariant into the
actual delete path (e.g., in the controller or User model deletion routine that
performs the delete), wrap the operation in a DB transaction and acquire a
pessimistic lock when counting (use the query with lockForUpdate()/SELECT ...
FOR UPDATE) to re-check the number of super admins before deleting, and
abort/throw an exception if the count would drop to zero; keep the policy for
quick UI hints but enforce correctness in the transactional delete code that
calls the model's delete method.
- Around line 92-99: The org-admin branch in UserPolicy currently authorizes any
pending user in the org, which lets an org admin act on a pending super-admin
account; update the return expression in the method (the block using
CurrentOrganization::class, isOrgAdmin(), and belongsToOrganization()) to also
require that the target $model is not a super-admin by adding a check like
!$model->isSuperAdmin() (or the equivalent role check on the User model) so org
admins cannot copy/claim super-admin invitation tokens.
In `@resources/views/layouts/app.blade.php`:
- Around line 38-39: Remove the unused resolution of $currentOrg: delete the
line that calls app(\App\Services\CurrentOrganization::class) since $currentOrg
is never used and SetCurrentOrganization middleware already initializes the
singleton; ensure no other logic depends on that resolved value and keep the
$showSwitcher line as-is (which uses $user->organizations()) to preserve
behavior.
In `@resources/views/livewire/configuration/organization.blade.php`:
- Around line 38-40: The Organization model's hasResources() currently issues
three exists() queries and should be refactored to use the preloaded withCount
attributes (database_servers_count, volumes_count, agents_count) to avoid the
N+1; update Organization::hasResources() to return true if any of
($this->database_servers_count ?? 0), ($this->volumes_count ?? 0), or
($this->agents_count ?? 0) are greater than zero so the Livewire component's
withCount(['databaseServers','volumes','agents']) is used instead of additional
exists() calls.
---
Outside diff comments:
In `@resources/views/layouts/app.blade.php`:
- Line 62: The "Super Admin" badge is using a static value attribute and not
going through the translation helper; update the badge component usage (the
element using value="Super Admin", e.g., the <x-badge> or similar component) to
use the translation helper and dynamic binding by replacing value="Super Admin"
with :value="__('Super Admin')" so the string is translatable and uses the :attr
binding pattern consistent with other strings in this file.
---
Nitpick comments:
In `@app/Livewire/Configuration/Organization.php`:
- Line 76: The redirect calls in the Livewire component Organization.php perform
full page loads; replace each plain redirect call (the return
$this->redirect(route('configuration.organizations')) instances at the three
spots) with Livewire navigation-enabled redirects by calling
$this->redirect(..., navigate: true) so the component uses SPA-style navigation;
update the redirect invocations in the methods that currently return redirects
(the Organization Livewire action methods around the redirect lines)
accordingly.
- Around line 65-67: The inline validation arrays for newOrgName (and similarly
editOrgName) should be moved out of the method and defined as Livewire
validation metadata — add #[Validate] attributes (or a small Form object) on the
Organization component properties newOrgName and editOrgName with the rules
'required|string|max:255|unique:organizations,name' (and the corresponding edit
rule), then remove the ad-hoc arrays from the methods (at lines calling
$this->validate([...])) so the methods simply call $this->validate() to trigger
the attribute-based validation; update any references in the create/edit
handlers to rely on the validated properties.
In `@app/Livewire/User/Create.php`:
- Around line 53-56: The inline validation for existingUserId and
existingUserRole inside the Livewire component should be moved to the
component's validation declarations instead of calling $this->validate() with an
ad-hoc array in addExisting(); update the component to use #[Validate]
attributes or include these fields on the existing UserForm so both invite and
addExisting() flows reuse the same rules (reference existingUserId,
existingUserRole, addExisting(), UserForm and #[Validate]) and remove the
duplicate inline rule array so validation follows the Livewire form/attribute
pattern.
In `@app/Livewire/User/Index.php`:
- Around line 147-161: Replace the separate whereRelation + conditional whereHas
with a single whereHas on the 'organizations' relation: remove the call to
whereRelation('organizations', ...) and instead call whereHas('organizations',
function($q) use ($currentOrg) { $q->where('organization_id',
$currentOrg->id()); if ($this->roleFilter !== '') { $q->where('role',
$this->roleFilter); } }); — this drops the raw SQL, keeps the organization
filter and only applies the role filter when set, preserving the existing
with('organizations') and search logic.
In `@resources/views/livewire/configuration/organization.blade.php`:
- Line 12: The external anchor tag rendering the "Learn more" link uses
target="_blank" but is missing rel="noopener noreferrer"; update the <a> element
in the organization Blade view (the anchor with class "link link-primary" and
text "Learn more") to include rel="noopener noreferrer" so the new tab cannot
access window.opener and the referrer is not leaked.
In `@resources/views/livewire/user/create.blade.php`:
- Around line 60-74: The UI renders two labels for the role field because a
manual <label> element is placed immediately before the <x-radio-card-group>
which also outputs its own label via the :label prop; remove one to avoid
duplicate text—either delete the manual <label class="label label-text...">{{
__('Role in current organization') }}</label> or remove the :label="__('Role')"
prop on the <x-radio-card-group> component (same change must be applied in the
duplicate block near the bottom of the file that mirrors this role field).
In `@tests/Feature/User/CreateTest.php`:
- Around line 87-115: The tests for the Create Livewire component rely on
User::factory()->create() implicitly attaching users to Organization::main();
make the membership explicit: in the "admin can add existing user" test ensure
the created $existingUser is detached from Organization::main() (e.g., call
$existingUser->organizations()->detach(Organization::main()->id) or sync to the
other org) before calling Livewire::test(Create::class), and in the "rejects
adding user already in organization" test explicitly attach the $existingUser to
Organization::main() with the expected role (e.g.,
$existingUser->organizations()->attach(Organization::main()->id, ['role' =>
'member'])) so the preconditions for Create::addExisting and the
roleIn(Organization::main()) assertions are obvious and stable.
In `@tests/Feature/User/EditTest.php`:
- Around line 51-57: The test currently creates non-admin users with
User::factory()->create(['role' => $role]) which bypasses factory states and
misses per-organization pivot role; update the test 'non-admin cannot edit
users' to use the factory states instead (e.g.
User::factory()->member()->create() and User::factory()->viewer()->create()) so
the per-organization pivot role is set correctly; keep the rest of the test (the
actingAs($user) call and get(route('users.edit', $target))->assertForbidden())
unchanged and ensure the ->with([...]) data provider values match the state
method names.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db754d77-afa1-4ed2-9f69-c7041244871c
📒 Files selected for processing (23)
app/Http/Middleware/SetCurrentOrganization.phpapp/Livewire/Configuration/Organization.phpapp/Livewire/User/Create.phpapp/Livewire/User/Index.phpapp/Models/User.phpapp/Policies/UserPolicy.phpapp/Providers/AppServiceProvider.phpapp/Services/CurrentOrganization.phpdocs/docs/user-guide/organizations.mdresources/views/layouts/app.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/livewire/user/index.blade.phptests/Feature/Api/BackupApiTest.phptests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/Api/OrganizationContextApiTest.phptests/Feature/Api/RestoreApiTest.phptests/Feature/Api/VolumeCrudApiTest.phptests/Feature/Configuration/OrganizationTest.phptests/Feature/User/CreateTest.phptests/Feature/User/EditTest.phptests/Feature/User/IndexTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Feature/Configuration/OrganizationTest.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (6)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phpapp/Http/Middleware/SetCurrentOrganization.phptests/Feature/Api/VolumeCrudApiTest.phpresources/views/livewire/configuration/organization.blade.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phpapp/Services/CurrentOrganization.phptests/Feature/Api/RestoreApiTest.phpresources/views/livewire/user/index.blade.phptests/Feature/User/IndexTest.phpapp/Providers/AppServiceProvider.phpapp/Livewire/User/Index.phpresources/views/layouts/app.blade.phptests/Feature/Api/OrganizationContextApiTest.phpresources/views/livewire/user/create.blade.phpapp/Livewire/User/Create.phpapp/Policies/UserPolicy.phpapp/Models/User.phpapp/Livewire/Configuration/Organization.phptests/Feature/User/CreateTest.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phpapp/Http/Middleware/SetCurrentOrganization.phptests/Feature/Api/VolumeCrudApiTest.phpresources/views/livewire/configuration/organization.blade.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phpapp/Services/CurrentOrganization.phptests/Feature/Api/RestoreApiTest.phpresources/views/livewire/user/index.blade.phptests/Feature/User/IndexTest.phpapp/Providers/AppServiceProvider.phpapp/Livewire/User/Index.phpresources/views/layouts/app.blade.phptests/Feature/Api/OrganizationContextApiTest.phpresources/views/livewire/user/create.blade.phpapp/Livewire/User/Create.phpapp/Policies/UserPolicy.phpapp/Models/User.phpapp/Livewire/Configuration/Organization.phptests/Feature/User/CreateTest.php
tests/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.php: When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model.
Use faker methods such as$this->faker->word()orfake()->randomDigit()when generating test data. Follow existing conventions on whether to use$this->fakerorfake().
Create Pest tests usingphp artisan make:test --pest {name}without including the test suite directory. Usephp artisan make:test --pest SomeFeatureTestinstead ofphp artisan make:test --pest Feature/SomeFeatureTest.
Files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phptests/Feature/Api/VolumeCrudApiTest.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phptests/Feature/Api/RestoreApiTest.phptests/Feature/User/IndexTest.phptests/Feature/Api/OrganizationContextApiTest.phptests/Feature/User/CreateTest.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
All Livewire component blade files should contain only view markup; all PHP logic must be in the component class.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/index.blade.phpresources/views/livewire/user/create.blade.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components in Mary UI should be prefixed withx-(e.g.,<x-button>,<x-input>,<x-card>) and use Heroicons for icons (e.g.,icon="o-user"for outline,icon="s-user"for solid).
In Mary UI select components, use the:optionsprop with array format[['id' => 'value', 'name' => 'Label']].
In Mary UI alerts, useclass="alert-success",class="alert-error", etc. (notvariantprop).
Use Alpine.js for client-side interactions in Livewire instead of JavaScript frameworks.
In Blade templates, use:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/index.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/user/create.blade.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/User/Index.phpapp/Livewire/User/Create.phpapp/Livewire/Configuration/Organization.php
🧠 Learnings (9)
📚 Learning: 2026-02-13T11:05:37.072Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 105
File: tests/Feature/Services/Backup/Databases/SqliteDatabaseTest.php:8-11
Timestamp: 2026-02-13T11:05:37.072Z
Learning: Adopt a global afterEach hook in tests/Pest.php (or equivalent Pest bootstrap) to clean up temporary directories created during tests. Specifically handle temp dirs named with the prefixes sqlite-db-test-*, backup-task-test-*, restore-task-test-*, and volume-test-*, so individual test files don’t need their own cleanup logic. This applies to all PHP test files under the tests directory.
Applied to files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phptests/Feature/Api/VolumeCrudApiTest.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phptests/Feature/Api/RestoreApiTest.phptests/Feature/User/IndexTest.phptests/Feature/Api/OrganizationContextApiTest.phptests/Feature/User/CreateTest.php
📚 Learning: 2026-04-09T13:59:25.873Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 205
File: tests/Feature/ConfigurationTest.php:165-176
Timestamp: 2026-04-09T13:59:25.873Z
Learning: In this repository, `Notification::fake()` is already performed globally in the base test class (`tests/TestCase.php`) within `setUp()` before each test. When reviewing individual test files under `tests/`, do not flag missing `Notification::fake()` calls, since they are handled by the shared base test setup.
Applied to files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phptests/Feature/Api/VolumeCrudApiTest.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phptests/Feature/Api/RestoreApiTest.phptests/Feature/User/IndexTest.phptests/Feature/Api/OrganizationContextApiTest.phptests/Feature/User/CreateTest.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
tests/Feature/Api/BackupScheduleCrudApiTest.phptests/Feature/Api/BackupApiTest.phpapp/Http/Middleware/SetCurrentOrganization.phptests/Feature/Api/VolumeCrudApiTest.phpresources/views/livewire/configuration/organization.blade.phptests/Feature/Api/DatabaseServerCrudApiTest.phptests/Feature/User/EditTest.phpapp/Services/CurrentOrganization.phptests/Feature/Api/RestoreApiTest.phpresources/views/livewire/user/index.blade.phptests/Feature/User/IndexTest.phpapp/Providers/AppServiceProvider.phpapp/Livewire/User/Index.phpresources/views/layouts/app.blade.phptests/Feature/Api/OrganizationContextApiTest.phpresources/views/livewire/user/create.blade.phpapp/Livewire/User/Create.phpapp/Policies/UserPolicy.phpapp/Models/User.phpapp/Livewire/Configuration/Organization.phptests/Feature/User/CreateTest.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Http/Middleware/SetCurrentOrganization.phpapp/Services/CurrentOrganization.phpapp/Providers/AppServiceProvider.phpapp/Livewire/User/Index.phpapp/Livewire/User/Create.phpapp/Policies/UserPolicy.phpapp/Models/User.phpapp/Livewire/Configuration/Organization.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/index.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/user/create.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/index.blade.phpresources/views/layouts/app.blade.phpresources/views/livewire/user/create.blade.php
📚 Learning: 2026-05-05T23:16:24.464Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Services/CurrentOrganization.php:28-39
Timestamp: 2026-05-05T23:16:24.464Z
Learning: Enforce non-nullable return types for id(): string and model(): Organization in PHP service classes under app/Services. Ensure SetCurrentOrganization middleware always resolves an Organization in the web context before these methods are reached; do not reintroduce isResolved guards in callers. A null dereference due to a broken middleware chain is the intended failure signal, not silent null propagation. Document the contract in class docblocks and consider tests to verify the middleware guarantees.
Applied to files:
app/Services/CurrentOrganization.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/User/Index.phpapp/Livewire/User/Create.phpapp/Livewire/Configuration/Organization.php
📚 Learning: 2026-03-28T22:33:16.333Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 195
File: app/Notifications/CLAUDE.md:7-12
Timestamp: 2026-03-28T22:33:16.333Z
Learning: In this repository (David-Crty/databasement), do not flag fenced Markdown code blocks that omit a language identifier (i.e., MD040: missing language after ```). The project treats unlabeled fenced code blocks as plain text by default, so missing language tags are acceptable for Markdown files.
Applied to files:
docs/docs/user-guide/organizations.md
🪛 markdownlint-cli2 (0.22.1)
docs/docs/user-guide/organizations.md
[warning] 65-65: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
docs/docs/user-guide/organizations.md (1)
1-109: Comprehensive and consistent organizations guide — looks good to merge.The page is clear, well-structured, and covers the key multi-organization behaviors and constraints (roles, scoping, switching, API usage, and isolation) in a way that matches the feature scope.
resources/views/livewire/user/index.blade.php (1)
49-67: PHP logic in Livewire blade and N+1 query concern.The
@phpblock (service resolution, role derivation, badge class computation) and the per-rowroleIn()DB call were both flagged in previous review rounds. Marking as duplicate to keep tracking.tests/Feature/Api/OrganizationContextApiTest.php (1)
7-130: Comprehensive org-context API test coverage — LGTM.The suite covers the full spectrum: default org scoping, explicit
org_idoverride,X-Organization-Idheader parity, cross-org isolation, 403 on inaccessible/nonexistent org ID, super-admin bypass, and multi-org switching. The 403 (not 404) for nonexistentorg_id(line 49-55) is correct from a security standpoint.tests/Feature/Api/BackupApiTest.php (1)
17-17: Factory state standardization — LGTM.Replacing the direct role attribute with
->viewer()factory state is the correct approach and consistent with the rest of the test suite changes in this PR.tests/Feature/Api/RestoreApiTest.php (1)
18-18: Factory state standardization — LGTM.tests/Feature/Api/BackupScheduleCrudApiTest.php (1)
57-57: Factory state standardization across all three mutation tests — LGTM.Also applies to: 127-127, 186-186
app/Providers/AppServiceProvider.php (1)
37-37: Singleton binding and Scramble org_id injection — LGTM.
singleton(CurrentOrganization::class)is correct for the standard PHP-FPM lifecycle; thewithDocumentTransformerspattern for injectingorg_idinto all operations is idiomatic for Scramble 0.12+.Also applies to: 104-112
tests/Feature/Api/VolumeCrudApiTest.php (1)
75-75: Factory state standardization and Mockery-based filesystem isolation — LGTM.Both the
viewer()factory state adoption and theMockery::mock(FilesystemProvider::class)approach for connection tests are clean improvements over the previous approach.Also applies to: 136-136
resources/views/livewire/user/create.blade.php (1)
51-57: PHP authorization logic still in template.
auth()->user()->isSuperAdmin()is invoked directly in the Blade. Per the coding guideline that Livewire blade files contain only view markup, this should be exposed as a public component property (e.g.,$isSuperAdminset inmount()) and referenced as@if($isSuperAdmin).As per coding guidelines: "All Livewire component blade files should contain only view markup; all PHP logic must be in the component class."
tests/Feature/User/CreateTest.php (1)
29-34: 💤 Low valuePrefer factory states over
['role' => $role].Same observation as in
IndexTest/EditTest: use the existingmember()/viewer()factory states so both the legacy column and the per-organization pivot role are set.As per coding guidelines: "Check if the factory has custom states before manually setting up the model."
tests/Feature/Api/DatabaseServerCrudApiTest.php (1)
17-17: LGTM — viewer factory state usage.Switching to
User::factory()->viewer()->create()matches the PR's per-organization role model and follows the project convention of using factory states.Also applies to: 242-242, 370-370
app/Http/Middleware/SetCurrentOrganization.php (1)
44-51: ⚡ Quick winThe strict comparison is safe.
CurrentOrganization::id()has an explicit return type ofstring, andOrganizationusesHasUlids(generating string-based ULIDs). Both values being compared on line 49 are strings, so the strict equality check is appropriate.> Likely an incorrect or invalid review comment.tests/Feature/User/IndexTest.php (1)
29-35: ⚡ Quick winThe proposed factory state approach won't work—there is no
member()state method.The UserFactory contains state methods for
viewer(),admin(),superAdmin(), anddemo(), but notmember(). The current test code usingUser::factory()->create(['role' => $role])is actually correct by design: the factory'snewModel()method intercepts the virtual 'role' attribute and stages it viapendingPivotRoleso that theconfigure()hook can attach the user to the organization with the intended role. The default definition already providesrole=>User::ROLE_MEMBER, so passing['role' => $role]as an attribute is the intended pattern for setting per-organization roles.If you want to use factory states, the
viewer()state exists and can be used for viewers, but you'd need either a newmember()state method added to UserFactory (out of scope here) or conditional test logic.> Likely an incorrect or invalid review comment.app/Models/User.php (1)
108-115: Thesuper_adminfield is safe to keep in$fillablefor this codebase.All user write paths properly gate the
super_adminfield behind application-level authorization checks:
- CreateNewUser (first registration): hardcodes
super_admin => true- UserForm (admin user management): conditions
super_adminassignment onauth()->user()->isSuperAdmin()check- AcceptInvitation (invitation flow): excludes
super_adminfrom the update- Profile settings: validates only
nameandsuper_adminNo API endpoints, request validation, or form request classes accept
super_adminfrom user input. The concern in the original review is mitigated by proper application-level controls, though removing the field from$fillableand usingforceFill()explicitly where needed would reduce long-term maintenance risk.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
resources/views/livewire/user/create.blade.php (1)
51-57: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMove the super-admin authorization check into the Livewire component
Line 51 calls
auth()->user()->isSuperAdmin()directly in the Blade view. Expose a component property (e.g.,$isSuperAdmin) and render with@if($isSuperAdmin)so the template stays presentation-only.♻️ Proposed fix
- `@if`(auth()->user()->isSuperAdmin()) + `@if`($isSuperAdmin) <x-checkbox wire:model="form.superAdmin" :label="__('Super Admin')" :hint="__('Super admins can access all organizations and manage global settings.')" /> `@endif`// app/Livewire/User/Create.php public bool $isSuperAdmin = false; public function mount(): void { $this->isSuperAdmin = auth()->user()->isSuperAdmin(); }As per coding guidelines: "All Livewire component blade files should contain only view markup; all PHP logic must be in the component class."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/user/create.blade.php` around lines 51 - 57, The Blade view is calling auth()->user()->isSuperAdmin() directly; move that logic into the Livewire component by adding a public bool property $isSuperAdmin on the User\Create component (e.g., class Create) and set it in mount(): $this->isSuperAdmin = auth()->user()->isSuperAdmin(); then update the view to use `@if`($isSuperAdmin) instead of auth()->user()->isSuperAdmin() so the template remains presentation-only.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Livewire/Configuration/Organization.php`:
- Line 76: The redirect calls in createOrganization(), updateOrganization(), and
deleteOrganization() use $this->redirect(route(...)) which forces a full page
reload; change each to return
$this->redirect(route('configuration.organizations'), ['navigate' => true]) so
Livewire performs an SPA-style navigate transition. Alternatively, instead of
redirecting, reset component state and invalidate the organizations cache by
calling unset($this->organizations) (or $this->reset(...) as appropriate) after
the mutation to avoid any redirect; update the three methods
(createOrganization, updateOrganization, deleteOrganization) accordingly.
---
Duplicate comments:
In `@resources/views/livewire/user/create.blade.php`:
- Around line 51-57: The Blade view is calling auth()->user()->isSuperAdmin()
directly; move that logic into the Livewire component by adding a public bool
property $isSuperAdmin on the User\Create component (e.g., class Create) and set
it in mount(): $this->isSuperAdmin = auth()->user()->isSuperAdmin(); then update
the view to use `@if`($isSuperAdmin) instead of auth()->user()->isSuperAdmin() so
the template remains presentation-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b0665648-3092-4528-bae7-14eb6d57da75
📒 Files selected for processing (6)
app/Livewire/Configuration/Organization.phpapp/Models/Organization.phpapp/Policies/UserPolicy.phpresources/views/layouts/app.blade.phpresources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.phpapp/Models/Organization.phpapp/Policies/UserPolicy.phpapp/Livewire/Configuration/Organization.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
All Livewire component blade files should contain only view markup; all PHP logic must be in the component class.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components in Mary UI should be prefixed withx-(e.g.,<x-button>,<x-input>,<x-card>) and use Heroicons for icons (e.g.,icon="o-user"for outline,icon="s-user"for solid).
In Mary UI select components, use the:optionsprop with array format[['id' => 'value', 'name' => 'Label']].
In Mary UI alerts, useclass="alert-success",class="alert-error", etc. (notvariantprop).
Use Alpine.js for client-side interactions in Livewire instead of JavaScript frameworks.
In Blade templates, use:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.phpapp/Models/Organization.phpapp/Policies/UserPolicy.phpapp/Livewire/Configuration/Organization.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/Configuration/Organization.php
🧠 Learnings (5)
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
resources/views/livewire/configuration/organization.blade.phpresources/views/livewire/user/create.blade.phpresources/views/layouts/app.blade.phpapp/Models/Organization.phpapp/Policies/UserPolicy.phpapp/Livewire/Configuration/Organization.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Models/Organization.phpapp/Policies/UserPolicy.phpapp/Livewire/Configuration/Organization.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Configuration/Organization.php
🔇 Additional comments (7)
resources/views/livewire/user/create.blade.php (1)
82-89:x-selectoptions wiring looks correctThe
:options="$availableUsers"binding matches the expected Mary UI options contract (id/name) and keeps option-building logic in the component.app/Models/Organization.php (1)
37-113: LGTM!The model is well typed, generics on relations are accurate, and
hasResources()correctly relies on preloadedwithCountattributes — all current callers (organizations(),confirmDelete(),deleteOrganization()) preload them withwithoutGlobalScope(OrganizationScope::class), so cross-org counts are accurate.resources/views/layouts/app.blade.php (2)
35-45: LGTM — past dead-code concern resolved.The previously flagged unused
$currentOrgresolution has been removed;$showSwitchershort-circuits onisSuperAdmin()so the count query only runs for regular multi-org users.
54-56: LGTM — Users menu correctly gated by policy.
@can('viewAny', \App\Models\User::class)properly defers toUserPolicy::viewAny, which now restricts access to admins (super or org). This keeps the sidebar consistent with the org-scoped permission model.app/Policies/UserPolicy.php (1)
8-117: LGTM — all previously flagged escalation paths are closed.
update()andcopyInvitationLink()correctly add the! $model->isSuperAdmin()guard on the org‑admin branch, thedelete()path is restricted to super admins with a last‑super‑admin safety check, andmanageOrgMembership()consistently applies the super‑admin OR org‑admin pattern viaCurrentOrganization. Authorization model is internally consistent.app/Livewire/Configuration/Organization.php (1)
44-53: LGTM — withCount/withoutGlobalScope correctly applied.The past issue with
OrganizationScopezeroing out cross-org counts is resolved by wrapping each scoped relation in a closure that strips the global scope, while leavingusers(unscoped) untouched. The same pattern is consistently applied inconfirmDelete()anddeleteOrganization().resources/views/livewire/configuration/organization.blade.php (1)
1-73: LGTM — guideline-compliant Mary UI markup.
x-prefixed components, Heroicons,__()translations,:attrbindings for translated strings, andwire:modelon modal booleans are all in line with the project guidelines. ThehasResources()/is_maingating in the actions cell mirrorsOrganizationPolicyconstraints, so the UI won't expose disabled actions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/Livewire/Configuration/Organization.php (1)
23-29: ⚡ Quick winMove the shared field rules onto the Livewire properties.
These inline rule arrays work, but they bypass the repo’s Livewire validation pattern and duplicate the non-unique parts of the rules. Put the shared
required|string|max:255checks on$newOrgName/$editOrgNamewith#[Validate](or a small Form object), then keep only the action-specificuniquerule inline.♻️ Minimal refactor
use Livewire\Attributes\Computed; use Livewire\Attributes\Title; +use Livewire\Attributes\Validate; use Livewire\Component; @@ - public string $newOrgName = ''; + #[Validate('required|string|max:255')] + public string $newOrgName = ''; @@ - public string $editOrgName = ''; + #[Validate('required|string|max:255')] + public string $editOrgName = ''; @@ $this->validate([ - 'newOrgName' => 'required|string|max:255|unique:organizations,name', + 'newOrgName' => 'unique:organizations,name', ]); @@ $this->validate([ - 'editOrgName' => 'required|string|max:255|unique:organizations,name,'.$org->id, + 'editOrgName' => 'unique:organizations,name,'.$org->id, ]);As per coding guidelines: "In Livewire components, use
#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data."Also applies to: 67-69, 100-102
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Configuration/Organization.php` around lines 23 - 29, Move the common "required|string|max:255" validation onto the Livewire properties by adding the #[Validate] attribute for $newOrgName and $editOrgName (or replace with a small Form object) so those checks are centralized; then remove those parts from the inline rule arrays in the create/update actions and leave only the action-specific "unique" constraint inline (e.g., in the methods that currently reference editingOrgId or create org); finally ensure each action calls $this->validate() before processing so validation runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Livewire/Configuration/Organization.php`:
- Around line 57-61: openCreateModal() and openEditModal() currently clear
fields but leave Livewire's validation error bag, causing stale validation
messages to appear when reopening the modal; call Livewire's resetValidation()
in both methods (in Organization::openCreateModal and
Organization::openEditModal) to clear the error bag when opening the modal so
the form starts clean.
---
Nitpick comments:
In `@app/Livewire/Configuration/Organization.php`:
- Around line 23-29: Move the common "required|string|max:255" validation onto
the Livewire properties by adding the #[Validate] attribute for $newOrgName and
$editOrgName (or replace with a small Form object) so those checks are
centralized; then remove those parts from the inline rule arrays in the
create/update actions and leave only the action-specific "unique" constraint
inline (e.g., in the methods that currently reference editingOrgId or create
org); finally ensure each action calls $this->validate() before processing so
validation runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 80eff8aa-1d40-455d-8392-3e2258449346
📒 Files selected for processing (2)
app/Livewire/Configuration/Organization.phpresources/views/livewire/configuration/organization.blade.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (5)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
app/Livewire/Configuration/Organization.phpresources/views/livewire/configuration/organization.blade.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/Configuration/Organization.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
app/Livewire/Configuration/Organization.phpresources/views/livewire/configuration/organization.blade.php
resources/views/livewire/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
All Livewire component blade files should contain only view markup; all PHP logic must be in the component class.
Files:
resources/views/livewire/configuration/organization.blade.php
resources/views/**/*.blade.php
📄 CodeRabbit inference engine (CLAUDE.md)
resources/views/**/*.blade.php: All UI components in Mary UI should be prefixed withx-(e.g.,<x-button>,<x-input>,<x-card>) and use Heroicons for icons (e.g.,icon="o-user"for outline,icon="s-user"for solid).
In Mary UI select components, use the:optionsprop with array format[['id' => 'value', 'name' => 'Label']].
In Mary UI alerts, useclass="alert-success",class="alert-error", etc. (notvariantprop).
Use Alpine.js for client-side interactions in Livewire instead of JavaScript frameworks.
In Blade templates, use:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters.
Files:
resources/views/livewire/configuration/organization.blade.php
🧠 Learnings (5)
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Configuration/Organization.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
app/Livewire/Configuration/Organization.phpresources/views/livewire/configuration/organization.blade.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Livewire/Configuration/Organization.php
📚 Learning: 2026-01-30T22:27:46.107Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 61
File: resources/views/livewire/volume/connectors/s3-config.blade.php:1-13
Timestamp: 2026-01-30T22:27:46.107Z
Learning: In Blade template files (any .blade.php) within the databasement project, allow using alert-info for informational content inside <x-alert> components. The guideline that permits alert-success and alert-error does not exclude using alert-info for informational purposes. Apply this consistently to all Blade components that render alerts; ensure semantic usage and accessibility.
Applied to files:
resources/views/livewire/configuration/organization.blade.php
📚 Learning: 2026-02-06T10:34:43.585Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 75
File: resources/views/livewire/backup-job/_filters.blade.php:36-40
Timestamp: 2026-02-06T10:34:43.585Z
Learning: In Blade template files, when creating compact inline filter controls, prefer using native <input type="checkbox"> elements with daisyUI classes (e.g., checkbox checkbox-warning checkbox-xs) over the Mary UI <x-checkbox> component. The <x-checkbox> component adds wrapper markup (e.g., <div><fieldset><label> with gap-3) that can break tight inline flex layouts. Use the native input approach for compact inline controls, but reserve <x-checkbox> for form fields that require labels, hints, and errors.
Applied to files:
resources/views/livewire/configuration/organization.blade.php
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 7
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
app/Livewire/Dashboard/LatestJobs.php (1)
58-71:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCross-org data leak:
getSelectedJobProperty()skips org scoping.
fetchJobs()is now correctly scoped viaBackupJob::forCurrentOrg(), butgetSelectedJobProperty()at line 64 uses a bareBackupJob::with([...])->find($this->selectedJobId)— theforCurrentOrg()scope is absent.forCurrentOrg()is a local scope, sofind()will happily return a job belonging to a different organization.Additionally,
$selectedJobIdis a public scalar property without#[Locked], so a user can either directly wire-mutate it or callviewLogs('<cross-org-job-uuid>')as a Livewire action with an arbitrary ID.🔒 Proposed fix
- return BackupJob::with([ + return BackupJob::forCurrentOrg()->with([ 'snapshot.databaseServer', 'snapshot.triggeredBy', 'restore.snapshot.databaseServer', 'restore.targetServer', 'restore.triggeredBy', ])->find($this->selectedJobId);Also, lock the scalar property against direct wire manipulation:
+ #[\Livewire\Attributes\Locked] public ?string $selectedJobId = null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Dashboard/LatestJobs.php` around lines 58 - 71, getSelectedJobProperty() currently queries BackupJob without org scoping and returns cross-org records; change the query to use the local scope (call BackupJob::forCurrentOrg() before with(...)->find(...)) so only jobs from the current org are returned, update any action methods like viewLogs($id) to also fetch via BackupJob::forCurrentOrg()->... when resolving an arbitrary ID, and protect the public scalar $selectedJobId by adding the #[Locked] attribute to the property declaration to prevent direct Livewire wire-mutation.app/Livewire/Dashboard/JobStatusGrid.php (1)
68-82:⚠️ Potential issue | 🟠 Major | ⚡ Quick winIDOR:
selectedJob()is not org-scoped — cross-org job details are exposed
jobs()was explicitly changed toBackupJob::forCurrentOrg()(confirmingforCurrentOrgis a named scope, not a global one).selectedJob()still usesBackupJob::with([...])->find($this->selectedJobId), which returns jobs from any organization. BecauseviewLogs(string $id)is a public Livewire action, an authenticated user from org A can call it with any UUID and read full job details—snapshot names, database server names,triggeredByuser—belonging to org B.🔒 Proposed fix
- return BackupJob::with([ + return BackupJob::forCurrentOrg()->with([ 'snapshot.databaseServer', 'snapshot.triggeredBy', 'restore.snapshot.databaseServer', 'restore.targetServer', 'restore.triggeredBy', ])->find($this->selectedJobId);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Dashboard/JobStatusGrid.php` around lines 68 - 82, selectedJob() returns a BackupJob from any org, allowing cross-org access; constrain it to the current org by using the existing forCurrentOrg scope when querying. Specifically, update the selectedJob() method to call BackupJob::forCurrentOrg()->with([...])->find($this->selectedJobId) (or ->findOrFail/firstWhere as appropriate) so that only jobs belonging to the current organization are returned; ensure any Livewire actions that rely on selectedJob() like viewLogs(string $id) continue to use the scoped result.app/Livewire/Dashboard/SnapshotsCard.php (1)
56-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPer-org lock doesn't prevent concurrent global verification runs — org context must be passed to the job.
The lock key is
verify-snapshot-files:{orgId}, correctly preventing the same org from double-triggering. However,VerifySnapshotFileJob::dispatch()receives no arguments. Inside a queue worker there is no HTTP request context, soCurrentOrganization(set bySetCurrentOrganizationmiddleware) is unavailable — the job cannot know which org to scope to.The
SnapshotVerificationService::run()method executes a global query (Snapshot::query()->whereNotNull('filename')->...) across all organizations with no org scoping. Two org admins can simultaneously acquire their own distinct lock keys and each dispatch a concurrent global verification run, which is strictly worse than the previous single global lock.The org ID should be passed to the job constructor so the worker can scope the snapshot query to the correct organization.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Dashboard/SnapshotsCard.php` around lines 56 - 66, The lock is per-organization but the dispatched job lacks org context, so update VerifySnapshotFileJob to accept and store the current organization id (from app(CurrentOrganization::class)->id()) when dispatching in SnapshotsCard, and modify the job handler to set the organization context (or pass the id down) before calling SnapshotVerificationService::run(); also change SnapshotVerificationService::run() to scope its Snapshot query by the provided org id instead of running a global query so workers only verify snapshots for the intended organization.tests/Feature/DatabaseServer/EditTest.php (1)
15-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse factories and guard
Organization::first()with null-safe operator.Two issues in these test setups:
Guideline violation —
Volume::create()andDatabaseServer::create()should be replaced with their respective factories. BothVolumeFactoryandDatabaseServerFactoryalready defaultorganization_id, so the explicitorganization_idargument is likely redundant.Potential
TypeError—Organization::first()->id(no null-safe?->) will throw ifOrganization::first()returnsnull. The factory pattern used throughout the codebase isOrganization::first()?->id ?? Organization::factory()->main().✅ Proposed fix (example for `can edit database server`)
- $volume = Volume::create([ - 'name' => 'Test Volume', - 'type' => 'local', - 'config' => ['path' => '/var/backups'], - 'organization_id' => \App\Models\Organization::first()->id, - ]); + $volume = Volume::factory()->local()->create(); ... - $serverData = [ - 'name' => $config['name'], - 'database_type' => $config['type'], - 'organization_id' => \App\Models\Organization::first()->id, - ]; + $serverData = [ + 'name' => $config['name'], + 'database_type' => $config['type'], + ];The same applies to the
can change retention policytest at lines 102–118.As per coding guidelines: "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
Also applies to: 102-118
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/EditTest.php` around lines 15 - 27, Replace direct model creation calls and unsafe Organization access with factories and null-safe lookup: stop using Volume::create(...) and DatabaseServer::create(...) in the tests and instantiate via VolumeFactory and DatabaseServerFactory (they already default organization_id), and change Organization::first()->id to the null-safe pattern Organization::first()?->id ?? Organization::factory()->main() so tests won’t TypeError when no Organization exists; apply the same changes in the can change retention policy test (lines indicated) and remove redundant explicit organization_id where factory defaults suffice.docs/docs/user-guide/permissions.md (1)
51-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDocumentation contradiction: "Delete user" permission listed for Admin but restricted to Super Admin.
The Users permissions table (line 56) shows Admin ✅ for "Delete user," but the Special Rules section (line 63) states "Only super admins can delete users." Based on the PR intent and AI summary, only Super Admins can delete users, making the table entry wrong.
📝 Proposed fix
-| Delete user | ❌ | ❌ | ✅ | +| Delete user | ❌ | ❌ | ❌ |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/user-guide/permissions.md` around lines 51 - 66, The permissions table is inconsistent with the Special Rules: "Delete user" is allowed only for Super Admins. Update the table header to include a Super Admin column (e.g., | Viewer | Member | Admin | Super Admin |) and change the "Delete user" row so Viewer, Member, and Admin show ❌ and Super Admin shows ✅; ensure the "Copy invitation link", "Invite new user", and "Edit user role" rows remain correct for Admin vs Super Admin as intended. Keep the rest of the table content and the "User Deletion Restrictions" text unchanged.docker-compose.yml (1)
169-177:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale "To enable" instructions — step 2 is now obsolete.
The
dexservice is already uncommented and active, but the comment above still says2) Uncomment this service. New contributors following these instructions will be confused. Step 2 should be removed (or the block reworded to describe the service as enabled by default).📝 Proposed fix
# Dex OIDC provider for local OAuth testing -# To enable: 1) Add "127.0.0.1 dex-local" to /etc/hosts -# 2) Uncomment this service -# 3) Set OAUTH_OIDC_ENABLED=true in .env.local +# Prerequisites: 1) Add "127.0.0.1 dex-local" to /etc/hosts +# 2) Set OAUTH_OIDC_ENABLED=true in .env.local🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 169 - 177, The comment above the Dex OIDC service is outdated — it still instructs contributors to "Uncomment this service" even though the dex service is enabled; update the docker-compose.yml comment block referring to the "dex" service by removing step 2 or rewording the "To enable" list to reflect that the dex service is enabled by default (e.g., remove the "Uncomment this service" item and adjust wording to "Enabled by default; to use set OAUTH_OIDC_ENABLED=true and add 127.0.0.1 dex-local to /etc/hosts"), ensuring references to the "dex" service and OAUTH_OIDC_ENABLED remain accurate.
🟡 Minor comments (14)
app/Livewire/User/Edit.php-47-47 (1)
47-47:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
auth()->user()is statically nullable — add null-safe operator.
auth()->user()returnsAuthenticatable|null. Calling->isSuperAdmin()directly will throw if the user is somehow unauthenticated during a re-render (e.g. expired session with an in-flight Livewire request).🛡️ Proposed fix
- 'isSuperAdmin' => auth()->user()->isSuperAdmin(), + 'isSuperAdmin' => auth()->user()?->isSuperAdmin() ?? false,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/User/Edit.php` at line 47, The code passes auth()->user()->isSuperAdmin() without guarding for a null user; change the expression to use the null-safe operator and a default boolean, e.g. auth()->user()?->isSuperAdmin() ?? false, so the 'isSuperAdmin' value is always a boolean and won't throw if auth()->user() is null; update the array entry where 'isSuperAdmin' is set to use this null-safe expression (replace the direct call to isSuperAdmin()).tests/Feature/Mcp/McpServerTest.php-196-196 (1)
196-196:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::first()without a null guard can throw in isolated test runs.If the test database contains no organizations when this line executes,
Organization::first()returnsnulland PHP will throw on->id. TheVolumeFactoryin this same PR correctly usesOrganization::first()?->idwith a fallback. Apply the same defensive pattern here (and at line 259).✏️ Proposed fix (lines 196 and 259)
- 'organization_id' => \App\Models\Organization::first()->id, + 'organization_id' => \App\Models\Organization::first()?->id + ?? \App\Models\Organization::factory()->main()->create()->id,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Mcp/McpServerTest.php` at line 196, The test sets 'organization_id' using Organization::first()->id which will crash if no Organization exists; change the assignment to use the null-safe operator and a fallback like Organization::first()?->id ?? Organization::factory()->create()->id so it returns an existing id or creates one; apply the same change to the other occurrence referenced in the review (the second 'organization_id' assignment).tests/Feature/DatabaseServer/SshTunnelTest.php-20-21 (1)
20-21:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
Organization::main()instead ofOrganization::first()to avoid null dereference.This file uses
RefreshDatabase, soOrganization::first()returnsnullif no org seeder runs, causing a fatalTypeErroron->id. Other tests in this PR (e.g.,SnapshotsCardTestline 98) consistently useOrganization::main(), which provides a guaranteed non-null result.✏️ Proposed fix (apply to all 4 occurrences)
-'organization_id' => \App\Models\Organization::first()->id, +'organization_id' => \App\Models\Organization::main()->id,Also applies to: 69-70, 113-114, 195-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/SshTunnelTest.php` around lines 20 - 21, Replace all uses of Organization::first() in tests/Feature/DatabaseServer/SshTunnelTest.php with Organization::main() to avoid null dereference under RefreshDatabase; specifically update the four occurrences where code builds test data (the array entries using 'organization_id' => \App\Models\Organization::first()->id) so they call \App\Models\Organization::main()->id instead, ensuring a guaranteed non-null Organization instance is returned for the tests.tests/Feature/Volume/VolumeTest.php-207-208 (1)
207-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSame
Organization::first()null-dereference risk asSshTunnelTest.php.All seven
Volume::create([..., 'organization_id' => \App\Models\Organization::first()->id])calls are subject to aTypeErrorif no organization row exists when the test runs. Replace withOrganization::main()->idto match the safe accessor used elsewhere in this PR.✏️ Proposed fix (apply to all 7 occurrences)
-'organization_id' => \App\Models\Organization::first()->id, +'organization_id' => \App\Models\Organization::main()->id,Also applies to: 213-214, 230-231, 236-237, 254-255, 408-409, 452-453
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Volume/VolumeTest.php` around lines 207 - 208, Replace the unsafe Organization::first()->id usage in the Volume::create calls with the safe accessor Organization::main()->id; locate each Volume::create([... 'organization_id' => \App\Models\Organization::first()->id]) occurrence (seven total) and change the organization_id expression to \App\Models\Organization::main()->id so it no longer risks a null-dereference at runtime.app/Livewire/Settings/TwoFactor.php-46-49 (1)
46-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale inline comment — update to match the broadened
isOAuth()check.The comment says "OAuth-only users" but the predicate was widened from
isOAuthOnly()toisOAuth(), which now covers all OAuth-linked accounts regardless of whether they also have a password. The user-facing message on line 48 already reflects the correct intent; the inline comment should be updated to match.✏️ Proposed fix
- // OAuth-only users don't have a password to use 2FA + // OAuth users authenticate via their provider and should not configure app-level 2FA if (Auth::user()->isOAuth()) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Settings/TwoFactor.php` around lines 46 - 49, Update the stale inline comment above the Auth::user()->isOAuth() check in the TwoFactor Livewire component to reflect that the predicate now applies to all OAuth-linked accounts (not just OAuth-only users); replace "OAuth-only users don't have a password to use 2FA" with a concise note like "OAuth-linked accounts may not be eligible for 2FA" or similar so the comment matches the widened isOAuth() semantics.config/oauth.php-24-24 (1)
24-24:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEnv var name breaks the
OAUTH_OIDC_*naming convention.Every other OIDC-specific setting in this file follows the
OAUTH_OIDC_prefix (OAUTH_OIDC_ENABLED,OAUTH_OIDC_CLIENT_ID,OAUTH_OIDC_ROLE_CLAIM, etc.). The new variableOIDC_DEFAULT_ORGANIZATION_IDis the only exception, which complicates discoverability for operators configuring the SSO block. Consider renaming toOAUTH_OIDC_DEFAULT_ORGANIZATION_IDand updating the corresponding documentation.✏️ Proposed fix
-'default_organization_id' => env('OIDC_DEFAULT_ORGANIZATION_ID'), +'default_organization_id' => env('OAUTH_OIDC_DEFAULT_ORGANIZATION_ID'),🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/oauth.php` at line 24, The env var used for default_organization_id breaks the OAUTH_OIDC_* convention; change the env key used in the config entry default_organization_id to read from env('OAUTH_OIDC_DEFAULT_ORGANIZATION_ID') instead of env('OIDC_DEFAULT_ORGANIZATION_ID'), and update any docs/config examples that reference the old OIDC_DEFAULT_ORGANIZATION_ID name to the new OAUTH_OIDC_DEFAULT_ORGANIZATION_ID so operators can discover it consistently with other OAUTH_OIDC_* settings.docs/docs/self-hosting/configuration/sso.md-141-149 (1)
141-149:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
OIDC_DEFAULT_ORGANIZATION_IDis missing from the Environment Variables Reference table and itsOIDC_prefix is misleading for non-OIDC OAuth providers.Two issues:
- The variable is not listed in the "Environment Variables Reference" table (lines 228–235), which only covers the OIDC role-mapping variables. Users scanning that table won't find it.
- The
OIDC_prefix (vs. theOAUTH_prefix used by all other user-creation settings likeOAUTH_DEFAULT_ROLE,OAUTH_AUTO_CREATE_USERS) implies this applies only to the Generic OIDC provider, but the description says it affects all auto-created OAuth users. This could cause Google/GitHub/GitLab OAuth users to overlook it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/self-hosting/configuration/sso.md` around lines 141 - 149, The docs omit OIDC_DEFAULT_ORGANIZATION_ID from the "Environment Variables Reference" table and its OIDC_ prefix misleads readers — add an entry for the organization-default env var to that table and change the documented variable name to OAUTH_DEFAULT_ORGANIZATION_ID (or clearly state that OIDC_DEFAULT_ORGANIZATION_ID is an alias and mark it deprecated) so it matches other user-creation vars like OAUTH_DEFAULT_ROLE and OAUTH_AUTO_CREATE_USERS; update the prose in the "Default Organization" section to mention the canonical name OAUTH_DEFAULT_ORGANIZATION_ID (and any alias handling) and ensure the table row includes a short description and example ULID.app/Models/BackupJob.php-78-92 (1)
78-92:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd
isResolved()guard before callingid()to matchOrganizationScope's pattern — and checkSnapshot.phpfor the same issue.
OrganizationScope::apply()guardsid()withisResolved()becauseCurrentOrganization::$organizationis nullable andid()directly accesses it without checks. Callingid()without the guard will throw "Call to a member function id() on null" in CLI contexts, queue workers, and tests without org setup.Both
BackupJob::scopeForCurrentOrg()(line 80) andSnapshot::scopeForCurrentOrg()(line 204) have the same unguarded call.🛡️ Proposed fix
public function scopeForCurrentOrg(Builder $query): Builder { - $orgId = app(CurrentOrganization::class)->id(); + $currentOrg = app(CurrentOrganization::class); + + if (! $currentOrg->isResolved()) { + return $query->whereRaw('0 = 1'); // No org context → no results + } + + $orgId = $currentOrg->id(); return $query->where(function (Builder $q) use ($orgId) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/BackupJob.php` around lines 78 - 92, The scopeForCurrentOrg methods call app(CurrentOrganization::class)->id() without checking if the current organization is resolved; add a guard using app(CurrentOrganization::class)->isResolved() and only call id() when true to avoid null dereference. Update BackupJob::scopeForCurrentOrg (and mirror the same change in Snapshot::scopeForCurrentOrg) to first get the CurrentOrganization instance, check isResolved(), and if not resolved return the query unmodified (or apply no organization filter), otherwise obtain id() and proceed with the whereHas/withoutGlobalScope logic.resources/views/livewire/settings/delete-user-form.blade.php-27-32 (1)
27-32:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
:labeldynamic binding for translated strings passed to component attributes.Lines 27, 31, and 32 pass translated strings via
{{ __() }}interpolation. This double-encodes HTML entities in translations.♻️ Proposed fix
- <x-password wire:model="password" label="{{ __('Password') }}" /> + <x-password wire:model="password" :label="__('Password')" /> <div class="flex justify-end gap-2"> - <x-button label="{{ __('Cancel') }}" `@click`="$wire.showDeleteModal = false" /> - <x-button label="{{ __('Delete account') }}" class="btn-error" type="submit" data-test="confirm-delete-user-button" /> + <x-button :label="__('Cancel')" `@click`="$wire.showDeleteModal = false" /> + <x-button :label="__('Delete account')" class="btn-error" type="submit" data-test="confirm-delete-user-button" /> </div>As per coding guidelines: "In Blade templates, use
:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/settings/delete-user-form.blade.php` around lines 27 - 32, The Blade template is passing translated strings into component attributes using interpolation (e.g., label="{{ __('Password') }}", label="{{ __('Cancel') }}", label="{{ __('Delete account') }}"), which can double-encode entities; update the component attribute bindings to use dynamic binding syntax (use :label) and pass the translation directly (e.g., :label="__('Password')" etc.) for the <x-password> and both <x-button> components (preserve other attributes like class, type, data-test unchanged).app/Http/Middleware/DemoModeMiddleware.php-55-68 (1)
55-68:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
Organization::main()is not null-safe — a missing main org crashes the demo login page.If
Organization::main()returnsnull(main org deleted, or demo mode enabled before migrations have run),->idon line 67 throws a fatal error, making the login page completely inaccessible.ensureDemoUserExists()is designed to be self-sufficient; mirror thefirstOrCreatepattern already used inCreateNewUser.php:🛡️ Proposed fix
- $user->organizations()->syncWithoutDetaching([ - Organization::main()->id => ['role' => UserRole::Demo->value], - ]); + $mainOrg = Organization::firstOrCreate( + ['is_main' => true], + ['name' => 'Main'] + ); + + $user->organizations()->syncWithoutDetaching([ + $mainOrg->id => ['role' => UserRole::Demo->value], + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Http/Middleware/DemoModeMiddleware.php` around lines 55 - 68, ensureDemoUserExists() assumes Organization::main() is non-null and calling ->id crashes if main org is missing; update the method to retrieve or create a main organization first (mirror the firstOrCreate pattern used in CreateNewUser.php) by calling Organization::firstOrCreate(...) with the same identifying attributes used for the main org, then use the returned Organization object's id when calling $user->organizations()->syncWithoutDetaching(...). Ensure you reference Organization::main() fallback to the newly created Organization so the demo user setup is self-sufficient if the main org is absent.tests/Feature/Settings/TwoFactorAuthenticationTest.php-31-38 (1)
31-38:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep a local password on this fixture so the test covers the new gate.
A passwordless OAuth account would also pass the old “OAuth-only” check, so this setup won’t catch a regression back to that behavior. Leaving the normal password in place and only adding the
OAuthIdentitymakes the assertion prove the broader block.Suggested change
-test('oauth user is blocked from two factor settings', function () { - $user = User::factory()->create(['password' => null]); +test('oauth user is blocked from two factor settings', function () { + $user = User::factory()->create(); OAuthIdentity::create([ 'user_id' => $user->id, 'provider' => 'github',🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Settings/TwoFactorAuthenticationTest.php` around lines 31 - 38, The test "oauth user is blocked from two factor settings" creates a passwordless user which doesn't exercise the new gate; instead keep a local password on the fixture by removing the ['password' => null] override (or explicitly set a hashed password via Hash::make) when calling User::factory()->create(), then attach the OAuthIdentity as before so the test proves an account with both a local password and an OAuthIdentity is blocked by the new gate.docs/docs/user-guide/organizations.md-67-73 (1)
67-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse the implemented
orgquery parameter in the API examples.This section documents
?org_id=, but the PR wiring and manual verification use?org=. Following the current example will make org scoping look broken to API users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/docs/user-guide/organizations.md` around lines 67 - 73, The docs currently show the query parameter as ?org_id= but the implementation and tests expect ?org=; update the example and surrounding text to use the implemented parameter (?org=01JA2B3C4D5E6F7G8H9J0KMNPQ) and confirm the alternate header (X-Organization-Id) wording remains accurate; ensure any mentions of org_id in this paragraph are replaced with org so the example in the GET /api/v1/database-servers request and the explanatory sentence match the actual behavior.app/Livewire/User/Create.php-101-105 (1)
101-105:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winClarify the scope and semantics of
hasMultipleOrganizations().This computed property checks the global organization count (
Organization::count() > 1), but the method name suggests it checks user-specific organizations. Since the component operates in a single-organization context (gating org-specific UI for "this organization"), either:
- Rename to
platformHasMultipleOrganizations()if the intent is to check whether the platform is multi-tenant, or- Change the implementation to
auth()->user()->organizations()->count() > 1if the intent is to check whether the current user belongs to multiple organizations.The current naming is ambiguous and the implementation may not match the intended use case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/User/Create.php` around lines 101 - 105, The computed property hasMultipleOrganizations() currently returns Organization::count() > 1 which checks platform-wide orgs but its name implies a user-scoped check; either rename the method to platformHasMultipleOrganizations() and keep the Organization::count() > 1 logic, or change its implementation to use the current user’s org membership (auth()->user()->organizations()->count() > 1) and keep the name hasMultipleOrganizations(); update any callers to match the chosen name/semantics (e.g., replace usages of hasMultipleOrganizations with platformHasMultipleOrganizations if you rename it).app/Models/User.php-205-208 (1)
205-208:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the return type annotation on
CurrentOrganization::model()to accurately reflect that the organization can be null.The middleware guarantees that requests with an unresolved organization are rejected before reaching code that calls
currentOrgRole(), socurrentOrgRole()is safe in practice. However,model()is annotated to returnOrganization(non-nullable) while the property$organizationisprivate ?Organization $organization = null. This type mismatch is misleading for static analysis and future maintainers.The return type should be
?Organizationto match the actual property type and the initialization state:/** * Get the current organization model. */ -public function model(): Organization +public function model(): ?Organization { return $this->organization; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/User.php` around lines 205 - 208, Change the return type of CurrentOrganization::model() from Organization to ?Organization to reflect the actual nullable property $organization; update the method signature and any associated docblock/PhpDoc in the CurrentOrganization class so static analysis matches runtime behavior, and ensure callers like User::currentOrgRole() (which calls app(\App\Services\CurrentOrganization::class)->model()) are satisfied by the nullable return type (adjust any strict type hints or null checks where necessary).
🧹 Nitpick comments (17)
resources/views/livewire/organization-switcher.blade.php (1)
6-7: 💤 Low valueRedundant
wire:model+wire:changecauses two Livewire round-trips per selectionOn
<select>elements, Livewire bindswire:modelto thechangeevent — the same eventwire:changeuses. Every org switch triggers two concurrent requests: one to updatecurrentOrgId, one to callswitchOrg().Consider one of the canonical Livewire patterns instead:
- Option A — keep
wire:model, dropwire:change, addupdatedCurrentOrgId($value)in the component class.- Option B — keep
wire:changeonly; set the initial selected state via a:valuebinding on the component.♻️ Option A (component-class hook)
- wire:model="currentOrgId" - wire:change="switchOrg($event.target.value)" + wire:model="currentOrgId"Then in
App\Livewire\OrganizationSwitcher:public function updatedCurrentOrgId(string $value): void { $this->switchOrg($value); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/organization-switcher.blade.php` around lines 6 - 7, The select currently uses both wire:model="currentOrgId" and wire:change="switchOrg($event.target.value)", causing duplicate Livewire requests; pick one pattern and remove the other. Either keep wire:model="currentOrgId" and remove wire:change, then add an updatedCurrentOrgId(string $value) method in the App\Livewire\OrganizationSwitcher class that calls switchOrg($value), or remove wire:model and keep wire:change="switchOrg($event.target.value)" and ensure the initial selected option is set from the component state (currentOrgId) so UI reflects the active org. Update the select and the OrganizationSwitcher component accordingly.resources/views/livewire/settings/profile.blade.php (1)
11-11: 💤 Low valueUse
:attrbinding for the translatedlabelto match project convention.
label="{{ __('Email') }}"passes a translated string via Blade echo. Per the coding guideline, pass translated strings to component attributes with:dynamic binding to avoid double-encoding issues in non-ASCII locales.✏️ Proposed fix
- label="{{ __('Email') }}" + :label="__('Email')"As per coding guidelines: "In Blade templates, use
:attrbinding (dynamic syntax) instead of{{ }}interpolation when passing translated strings to component attributes to avoid double-encoding special characters."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/settings/profile.blade.php` at line 11, The label attribute is using Blade echo which can double-encode translated text; update the component attribute from label="{{ __('Email') }}" to use dynamic binding :label="__('Email')" so the translated string is passed directly (look for the label attribute on the input/component in profile.blade.php and replace the quoted Blade interpolation with the :label dynamic binding).tests/Feature/Mcp/McpServerTest.php (1)
98-98: ⚡ Quick winUse
viewer()factory state to match the approach adopted in other test files.
DatabaseServerCrudApiTest.phpwas explicitly migrated fromcreate(['role' => 'viewer'])toUser::factory()->viewer()->create()as part of this PR. The same tests here were only half-updated (value changed toUserRole::Viewerenum, but the attribute-override approach retained). If therolecolumn is eventually removed fromusersin favour of the org pivot exclusively, this test will silently create a non-viewer user and the authorization assertion will pass for the wrong reasons.✏️ Proposed fix (lines 98 and 144)
- $user = User::factory()->create(['role' => UserRole::Viewer]); + $user = User::factory()->viewer()->create();As per coding guidelines: "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Mcp/McpServerTest.php` at line 98, Replace the manual role override when creating test users with the factory state to ensure the created user has the viewer traits; specifically change the creation in McpServerTest from using User::factory()->create(['role' => UserRole::Viewer]) to using the viewer factory state (User::factory()->viewer()->create()) so the test relies on the factory's viewer configuration rather than directly setting the role column; update any other occurrences in this file (e.g., the second creation at line ~144) to the same pattern to prevent future drift if the role column is removed.resources/views/livewire/configuration/_tabs.blade.php (1)
18-23: ⚡ Quick winMove
isSuperAdmin()check to the component class.Calling
auth()->user()->isSuperAdmin()inside a Livewire blade partial introduces PHP logic that should live in the component. This also creates a latent null-dereference ifauth()->user()ever returnsnull(e.g., middleware misconfiguration).✏️ Proposed fix
In the hosting Livewire component(s) that include this partial, expose the flag:
// e.g., app/Livewire/Configuration/SomePage.php public bool $showOrganizationsTab = false; public function mount(): void { $this->showOrganizationsTab = auth()->user()?->isSuperAdmin() ?? false; }Then in the partial:
- `@if`(auth()->user()->isSuperAdmin()) + `@if`($showOrganizationsTab) <a href="{{ route('configuration.organizations') }}" wire:navigate role="tab" `@class`(['tab', 'tab-active' => $active === 'organizations'])> {{ __('Organizations') }} </a> `@endif`As per coding guidelines: "All Livewire component blade files should contain only view markup; all PHP logic must be in the component class."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@resources/views/livewire/configuration/_tabs.blade.php` around lines 18 - 23, Move the isSuperAdmin() check out of the blade and into the Livewire component: add a public bool property (e.g., showOrganizationsTab) to the hosting component class and set it in mount() using a null-safe check on auth()->user() (e.g., auth()->user()?->isSuperAdmin() ?? false) so you avoid null dereference; then update the partial to use `@if`($showOrganizationsTab) instead of calling auth()->user()->isSuperAdmin() directly.app/Livewire/Forms/VolumeForm.php (1)
99-107: 💤 Low valueInline
organization_iddirectly into$datafor cleaner construction.The two-step build (array literal + separate append) is functionally equivalent but slightly harder to scan at a glance.
✏️ Proposed refactor
- $data = [ - 'name' => $this->name, - 'type' => $this->type, - 'config' => $this->buildConfig(), - ]; - - $data['organization_id'] = app(CurrentOrganization::class)->id(); - - Volume::create($data); + Volume::create([ + 'name' => $this->name, + 'type' => $this->type, + 'config' => $this->buildConfig(), + 'organization_id' => app(CurrentOrganization::class)->id(), + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Forms/VolumeForm.php` around lines 99 - 107, The array construction for creating a Volume is split into an initial $data literal and a later assignment of organization_id; inline the organization id directly into the initial $data array to improve readability. Update the block that builds $data (which currently sets 'name' => $this->name, 'type' => $this->type, 'config' => $this->buildConfig()) to include 'organization_id' => app(CurrentOrganization::class)->id(), then call Volume::create($data) as before so Volume::create, buildConfig, $this->name, $this->type and app(CurrentOrganization::class)->id() remain unchanged.tests/Feature/Dashboard/SnapshotsCardTest.php (1)
83-83: ⚡ Quick winUse
User::factory()->admin()->create()instead of manually setting the role attribute.The
UserFactoryhas a dedicatedadmin()state (line 93 ofdatabase/factories/UserFactory.php) that should be used per coding guidelines. Replace the manual role attribute assignments with the factory state method for consistency and clarity.Also applies to: 96-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Dashboard/SnapshotsCardTest.php` at line 83, Replace manual role assignment via User::factory()->create(['role' => UserRole::Admin]) with the factory state helper User::factory()->admin()->create() (and similarly replace the other occurrence noted) so the test uses the dedicated admin() state from UserFactory; update the instances in SnapshotsCardTest (where the variable $admin is created) to call admin() on the factory instead of passing the role attribute.app/Models/DatabaseServer.php (1)
22-66: ⚡ Quick winMissing
@propertyannotations for the neworganization_idfield andorganizationrelation.The class-level PHPDoc block doesn't declare the new column or relationship, which breaks IDE type inference and may cause PHPStan to miss type errors in callers that access
$server->organization_idor$server->organization.🔧 Proposed additions
* `@property` string|null $agent_id * `@property` string|null $managed_by + * `@property` string $organization_id * `@property` NotificationTrigger $notification_trigger ... * `@property-read` Agent|null $agent + * `@property-read` Organization $organization * `@property-read` Collection<int, Backup> $backups🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/DatabaseServer.php` around lines 22 - 66, The class PHPDoc for DatabaseServer is missing annotations for the new organization relationship and foreign key; update the docblock at the top of the DatabaseServer class to add a `@property` string|null $organization_id (or correct type) and a `@property-read` Organization|null $organization entry so IDEs and static analysis (PHPStan) recognize $server->organization_id and $server->organization; ensure the names match the actual column and relation method (organization) in the DatabaseServer model.tests/Feature/Auth/RegistrationTest.php (1)
29-31: 💤 Low valueConsider importing
Organizationto avoid the inline FQN.Line 31 uses
\App\Models\Organization::main()as an inline fully-qualified name. Adding ause App\Models\Organization;import at the top keeps it consistent with the file's other model imports.♻️ Proposed change
+use App\Models\Organization; use App\Enums\UserRole; use App\Models\DatabaseServer; use App\Models\User;- expect($user->super_admin)->toBeTrue() - ->and($user->roleIn(\App\Models\Organization::main()))->toBe(UserRole::Admin); + expect($user->super_admin)->toBeTrue() + ->and($user->roleIn(Organization::main()))->toBe(UserRole::Admin);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Auth/RegistrationTest.php` around lines 29 - 31, The test uses an inline FQN \App\Models\Organization::main() in RegistrationTest (lines referencing auth()->user() and UserRole::Admin); add a top-level import "use App\Models\Organization;" and replace the inline FQN with Organization::main() to match other model imports and keep imports consistent across the file.app/Models/Snapshot.php (1)
202-210: ⚡ Quick winSame missing
isResolved()guard asBackupJob::scopeForCurrentOrg.
id()is called directly without checkingisResolved()first.OrganizationScope::apply()always guardsid()— calling it unguarded may produce silent zero-row results (ifid()returnsnull) or throw in non-HTTP contexts.🛡️ Proposed fix
public function scopeForCurrentOrg(Builder $query): Builder { - $orgId = app(CurrentOrganization::class)->id(); + $currentOrg = app(CurrentOrganization::class); + + if (! $currentOrg->isResolved()) { + return $query->whereRaw('0 = 1'); // No org context → no results + } + + $orgId = $currentOrg->id(); return $query->whereHas('databaseServer', function (Builder $sq) use ($orgId) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Snapshot.php` around lines 202 - 210, scopeForCurrentOrg calls app(CurrentOrganization::class)->id() without first checking isResolved(), which can return null in non-HTTP contexts; mirror the fix used in BackupJob::scopeForCurrentOrg: call app(CurrentOrganization::class)->isResolved() and if it is not resolved return a query that yields no results (e.g., $query->whereRaw('0 = 1')), otherwise retrieve the org id and run the existing whereHas on the databaseServer relationship (preserving the withoutGlobalScope(OrganizationScope::class) and whereRaw('organization_id = ?', [$orgId])).tests/Feature/Services/Backup/LocalVolumeStorageTest.php (1)
38-56: ⚡ Quick winPrefer
Volume::factory()->create([...])over rawVolume::create()withOrganization::first().Same guideline violation as in
CreateTest.php. Even when overridingconfig, the factory pattern is preferred and avoids the fragileOrganization::first()call.♻️ Proposed refactor
- $volumeWithRoot = Volume::create([ - 'name' => 'Volume with root key', - 'type' => 'local', - 'config' => ['root' => $tempDir], - 'organization_id' => \App\Models\Organization::first()->id, - ]); + $volumeWithRoot = Volume::factory()->local()->create([ + 'name' => 'Volume with root key', + 'config' => ['root' => $tempDir], + ]); ... - $volumeWithPath = Volume::create([ - 'name' => 'Volume with path key', - 'type' => 'local', - 'config' => ['path' => $tempDir], - 'organization_id' => \App\Models\Organization::first()->id, - ]); + $volumeWithPath = Volume::factory()->local()->create([ + 'name' => 'Volume with path key', + 'config' => ['path' => $tempDir], + ]);As per coding guidelines: "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Services/Backup/LocalVolumeStorageTest.php` around lines 38 - 56, Tests are creating Volume models with Volume::create([...]) and calling Organization::first(), which violates the factory-first guideline; replace these Volume::create(...) usages with Volume::factory()->create(...) and pass the overridden attributes (name, type, config, organization_id) to the factory call (use Organization factory to create/get an organization via Organization::factory()->create() or reuse a created org) so the test uses model factories instead of direct creates; update the instances around Volume::create for $volumeWithRoot and $volumeWithPath and any related assertions that rely on those variables.tests/Feature/DatabaseServer/CreateTest.php (1)
13-18: ⚡ Quick winUse
Volume::factory()instead of rawVolume::create()withOrganization::first().This pattern repeats across all
Volumesetups in this file (Lines 13, 106, 145, 264, 296, 313, 330, 352, 429, 470, 476). Two issues:
- Guideline violation – tests should create models via factories, not raw
create().- Fragility –
Organization::first()returns?Organization; if the Pest bootstrap ever stops seeding an org, every->idcall throws a PHP fatal error with no descriptive failure.The Volume factory already has a
local()state; use it with an org context:♻️ Proposed refactor
- $volume = Volume::create([ - 'name' => 'Test Volume', - 'type' => 'local', - 'config' => ['path' => '/var/backups'], - 'organization_id' => \App\Models\Organization::first()->id, - ]); + $volume = Volume::factory()->local()->create();If the factory doesn't yet carry an
organization_idfrom the global test context, a small helper intests/Pest.php(e.g.currentOrganization()) or an->withOrganization()factory state removes the repeatedOrganization::first()call entirely.As per coding guidelines: "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/DatabaseServer/CreateTest.php` around lines 13 - 18, Replace direct Volume::create([...]) + Organization::first()->id with the Volume factory: use Volume::factory()->local()->for($org, 'organization')->create() where $org is provided by a test helper (e.g. currentOrganization()) or an Organization factory; alternatively add/use a factory state like ->withOrganization() to attach the organization. Update each occurrence (the Volume::create calls) to use Volume::factory(), the local() state, and .for(...) or the new state to eliminate Organization::first() and avoid null fatal errors.app/Models/DatabaseServerSshConfig.php (1)
15-29: 💤 Low valuePHPDoc block missing
organization_idandorganizationentries.Same gap as
Agent.php— the existing PHPDoc documents all fields and relations, so the new ones should be added for consistency and IDE support.📝 Proposed additions
* `@property` string|null $key_passphrase * `@property` Carbon|null $created_at * `@property` Carbon|null $updated_at + * `@property` string $organization_id * `@property-read` Collection<int, DatabaseServer> $databaseServers + * `@property-read` Organization $organization🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/DatabaseServerSshConfig.php` around lines 15 - 29, The PHPDoc for the DatabaseServerSshConfig model is missing the organization relation and foreign key—update the docblock in the DatabaseServerSshConfig class to add the organization_id property and the organization relation (e.g., add a `@property` string $organization_id and a `@property-read` Organization $organization) so IDEs and static analyzers can see the relation; ensure the class name DatabaseServerSshConfig and the existing `@property-read` Collection<int, DatabaseServer> $databaseServers stay intact.app/Models/Agent.php (1)
17-38: 💤 Low valuePHPDoc block is missing entries for the new
organization_idfield andorganizationrelation.The existing class-level PHPDoc documents every property and relation. The new additions lack corresponding entries.
📝 Proposed additions
* `@property` Carbon|null $updated_at + * `@property` string $organization_id * `@property-read` Collection<int, DatabaseServer> $databaseServers * `@property-read` int|null $database_servers_count * `@property-read` Collection<int, AgentJob> $agentJobs * `@property-read` int|null $agent_jobs_count + * `@property-read` Organization $organization🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/Agent.php` around lines 17 - 38, Update the class-level PHPDoc in Agent.php to document the new organization fields and relation: add a `@property` (string|null) $organization_id entry and a `@property-read` Organization $organization (and optionally `@property-read` int|null $organization_count if you expose a count) so the new organization_id attribute and organization() relation are reflected alongside the existing properties and relations.tests/Feature/Auth/OAuthTest.php (1)
96-99: ⚡ Quick winStop seeding raw
roleattributes in these fixtures.This PR moves role state to the
organization_userpivot, but these tests still passroleintoUser::factory()->create(). Please seed the org membership/role through the factory helper or by attaching the target organization explicitly, so the setup matches whatroleIn(...)actually reads.As per coding guidelines, "When creating models for tests, use factories. Check if the factory has custom states before manually setting up the model."
Also applies to: 343-343, 364-364, 520-520, 542-542
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Feature/Auth/OAuthTest.php` around lines 96 - 99, The test is incorrectly seeding the user's role via User::factory()->create(['role' => ...]) even though role is stored on the organization_user pivot and read via roleIn(...); update the setup for $existingUser (and the other occurrences) to remove the raw 'role' attribute from User::factory()->create() and instead assign the role through the org membership: either use an available factory helper/state that creates the organization membership with the desired role (e.g. a custom User factory state like withOrganizationRole or the project’s helper used elsewhere), or create the Organization and attach the user to it by creating an organization_user pivot record with the appropriate role so roleIn($org) will reflect the correct role for $existingUser.app/Livewire/Settings/DeleteUserForm.php (1)
28-28: ⚡ Quick winUse Livewire SPA redirect convention here.
Line 28 uses
navigate: false; project convention requires returning$this->redirect(..., navigate: true)for in-app navigation consistency.Proposed change
- $this->redirect(route('login'), navigate: false); + return $this->redirect(route('login'), navigate: true);As per coding guidelines, in Livewire components you should return
$this->redirect()withnavigate: truefor SPA-like navigation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/Settings/DeleteUserForm.php` at line 28, In the DeleteUserForm Livewire component replace the current SPA redirect call that uses navigate: false with the Livewire SPA convention: return the redirect and set navigate: true; specifically change the $this->redirect(route('login'), navigate: false) invocation in class DeleteUserForm (method performing the post-delete redirect) to return $this->redirect(route('login'), navigate: true) so the component performs in-app navigation correctly.app/Livewire/OrganizationSwitcher.php (1)
19-31: ⚡ Quick winReturn the Livewire redirect instead of discarding it.
This action currently calls
$this->redirect(...)but keeps avoidsignature and drops the return value. The repo convention for Livewire components is to return the redirect response when usingnavigate: true. As per coding guidelines: "In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Livewire/OrganizationSwitcher.php` around lines 19 - 31, The switchOrg method currently has a void signature and discards the Livewire redirect; change its signature to return the redirect response and return the $this->redirect(...) call (with navigate: true) instead of dropping it. Locate switchOrg (uses Organization::findOrFail, auth()->user(), and app(CurrentOrganization::class)->switchTo) and update the method return type and final statement to return the redirect so the Livewire SPA navigation behavior is preserved.app/Models/User.php (1)
118-119: 💤 Low value
pendingPivotRoleis exposed aspublic— consider tightening visibility.This is a transient hand-off slot from
UserFactoryto a creation hook, but as apublicproperty it can also be read/written by any caller (controllers, Livewire, blade) and is not guarded against accidental mutation. If the contract really is "factory-only,"protected(with a setter) or even moving the role hand-off to factory state metadata would communicate intent better and keep the model surface area lean.Not blocking — just a nudge for the data-flow shape.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Models/User.php` around lines 118 - 119, The public transient property User::$pendingPivotRole should be tightened: change its visibility to protected and add a minimal factory-only setter method (e.g., User::setPendingPivotRole(UserRole $role): void) that UserFactory calls to pass the pivot role, or alternatively move the hand-off into the factory's state metadata instead of storing it on the model; update references in UserFactory to use the new setter (or factory metadata) and keep the property non-public to prevent accidental external reads/writes.
a30ad5b to
e0da39c
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
app/Mcp/Tools/ListOrganizationsTool.php (1)
30-35: 💤 Low valueConsider an arrow function with a typed parameter for the
mapclosure.The closure parameter
$orgis untyped, which reduces static-analysis coverage.♻️ Proposed refactor
- $lines = $orgs->map(function ($org) use ($currentOrgId) { + $lines = $orgs->map(fn (\App\Models\Organization $org) => [ $active = $org->id === $currentOrgId ? ' **(active)**' : ''; $role = $org->pivot->role ?? 'unknown'; - return "- **{$org->name}** (ID: {$org->id}){$active}\n Role: {$role}".($org->is_main ? ' | Main organization' : ''); - }); + "- **{$org->name}** (ID: {$org->id}){$active}\n Role: {$role}".($org->is_main ? ' | Main organization' : '') + ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Mcp/Tools/ListOrganizationsTool.php` around lines 30 - 35, The map closure should use an arrow function with a typed parameter to improve static-analysis: change the anonymous function at $orgs->map(...) to an arrow fn with a type-hinted parameter (e.g. fn(\App\Models\Organization $org) => ...) and adjust the body accordingly, relying on arrow capture for $currentOrgId instead of use(...); add or update the Organization import/fully-qualified name if needed so the type resolves.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Mcp/Tools/ListOrganizationsTool.php`:
- Around line 19-22: The handle() method in ListOrganizationsTool should guard
against null $request->user() and a missing CurrentOrganization instance when
invoked via local (STDIO) transport; add an early check in
ListOrganizationsTool::handle() that if $request->user() is null or
app(CurrentOrganization::class) is not bound/returns null then return an empty
collection (or a safe default) instead of calling
app(CurrentOrganization::class)->id() or $user->organizations(); ensure you
reference the existing variables ($user) and calls ($request->user(),
app(CurrentOrganization::class)->id(), $user->organizations()) when implementing
the null checks so the method never dereferences null.
In `@app/Services/Backup/SnapshotVerificationService.php`:
- Around line 38-41: The closure passed to whereHas lacks a type hint for its
parameter $sq; update the closure in SnapshotVerificationService
whereHas('databaseServer', function ($sq) use ($organizationId) { ... }) to
declare the parameter type (e.g., function (Builder $sq) use ($organizationId) {
... }) so static analysis and PHP typing rules are satisfied; ensure you import
or reference the correct Builder/Query builder class used by databaseServer and
keep the existing withoutGlobalScope(OrganizationScope::class) and whereRaw call
unchanged.
In `@app/Services/OAuthService.php`:
- Around line 185-202: The default role resolution must be constrained to only
assignable roles: after deriving $role via
UserRole::tryFrom(config('oauth.default_role', 'member')) (or using
$resolvedRole), validate that the resulting UserRole is one of
UserRole::assignable() and if not fall back to a safe default (e.g.,
UserRole::Member); update the logic around $role in the OAuth user creation path
(near resolveDefaultOrganization and the organizations()->attach call) so
organizations()->attach($org->id, ['role' => $role->value]) only ever persists
an assignable enum value.
In `@docs/docs/user-guide/organizations.md`:
- Around line 67-73: Update the docs example and text to use the
active-organization contract `org` instead of `org_id`: change the query example
GET /api/v1/database-servers?org_id=... to GET /api/v1/database-servers?org=...,
and ensure the surrounding sentence refers to the `?org=` query parameter
(keeping `X-Organization-Id` header unchanged); edit the snippet in
organizations.md (the code block and the sentence mentioning `?org_id=`) so the
docs match the middleware and generated API docs.
---
Nitpick comments:
In `@app/Mcp/Tools/ListOrganizationsTool.php`:
- Around line 30-35: The map closure should use an arrow function with a typed
parameter to improve static-analysis: change the anonymous function at
$orgs->map(...) to an arrow fn with a type-hinted parameter (e.g.
fn(\App\Models\Organization $org) => ...) and adjust the body accordingly,
relying on arrow capture for $currentOrgId instead of use(...); add or update
the Organization import/fully-qualified name if needed so the type resolves.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 873439b9-fbd6-41a6-9dcb-40c34ac41d8b
📒 Files selected for processing (30)
README.mdSECURITY.mdapp/Http/Controllers/Api/V1/UserOrganizationController.phpapp/Http/Middleware/SetCurrentOrganization.phpapp/Http/Resources/OrganizationResource.phpapp/Jobs/VerifySnapshotFileJob.phpapp/Livewire/Configuration/Authentication.phpapp/Livewire/Configuration/Backup.phpapp/Livewire/Dashboard/JobStatusGrid.phpapp/Livewire/Dashboard/LatestJobs.phpapp/Livewire/Dashboard/SnapshotsCard.phpapp/Mcp/Servers/DatabasementServer.phpapp/Mcp/Tools/ListOrganizationsTool.phpapp/Models/Organization.phpapp/Policies/UserPolicy.phpapp/Queries/BackupJobQuery.phpapp/Services/Backup/SnapshotVerificationService.phpapp/Services/OAuthService.phpconfig/oauth.phpdatabase/factories/UserFactory.phpdatabase/migrations/2026_05_05_000002_backfill_organizations_data.phpdocker-compose.ymldocs/docs/self-hosting/configuration/sso.mddocs/docs/user-guide/organizations.mddocs/docs/user-guide/permissions.mdroutes/ai.phproutes/api.phptests/Feature/Api/UserOrganizationApiTest.phptests/Feature/Configuration/AuthenticationTest.phptests/Feature/Mcp/McpServerTest.php
✅ Files skipped from review due to trivial changes (5)
- SECURITY.md
- README.md
- tests/Feature/Api/UserOrganizationApiTest.php
- database/migrations/2026_05_05_000002_backfill_organizations_data.php
- app/Policies/UserPolicy.php
🚧 Files skipped from review as they are similar to previous changes (11)
- app/Livewire/Configuration/Authentication.php
- tests/Feature/Configuration/AuthenticationTest.php
- app/Queries/BackupJobQuery.php
- docker-compose.yml
- app/Livewire/Dashboard/SnapshotsCard.php
- app/Livewire/Dashboard/JobStatusGrid.php
- app/Http/Middleware/SetCurrentOrganization.php
- app/Livewire/Dashboard/LatestJobs.php
- database/factories/UserFactory.php
- tests/Feature/Mcp/McpServerTest.php
- app/Models/Organization.php
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: ci
🧰 Additional context used
📓 Path-based instructions (4)
**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.php: In PHP files, always use curly braces for control structures, even for single-line bodies.
Use PHP 8 constructor property promotion:public function __construct(public GitHub $github) { }. Do not leave empty zero-parameter__construct()methods unless the constructor is private.
Use explicit return type declarations and type hints for all method parameters in PHP:function isAccessible(User $user, ?string $path = null): bool
Prefer PHPDoc blocks over inline comments. Only add inline comments for exceptionally complex logic.
Use array shape type definitions in PHPDoc blocks for PHP.
Files:
routes/ai.phpapp/Http/Controllers/Api/V1/UserOrganizationController.phpapp/Jobs/VerifySnapshotFileJob.phpconfig/oauth.phpapp/Mcp/Tools/ListOrganizationsTool.phpapp/Services/Backup/SnapshotVerificationService.phpapp/Livewire/Configuration/Backup.phpapp/Mcp/Servers/DatabasementServer.phproutes/api.phpapp/Services/OAuthService.phpapp/Http/Resources/OrganizationResource.php
**/*.{php,blade.php}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{php,blade.php}: Always use named routes and theroute()function when generating links to other pages.
Translations should use__('...')helper function and be stored in JSON translation files inlang/{locale}.json. Keep technical terms like 'Backup', 'Restore', 'Snapshot(s)' in English across all locales.
Files:
routes/ai.phpapp/Http/Controllers/Api/V1/UserOrganizationController.phpapp/Jobs/VerifySnapshotFileJob.phpconfig/oauth.phpapp/Mcp/Tools/ListOrganizationsTool.phpapp/Services/Backup/SnapshotVerificationService.phpapp/Livewire/Configuration/Backup.phpapp/Mcp/Servers/DatabasementServer.phproutes/api.phpapp/Services/OAuthService.phpapp/Http/Resources/OrganizationResource.php
app/Livewire/**/*.php
📄 CodeRabbit inference engine (CLAUDE.md)
app/Livewire/**/*.php: In Livewire components, use#[Validate]attributes or Form objects for validation. Call$this->validate()before processing data.
In Livewire components, useSession::flash()for one-time messages and show them via@if (session('success'))in Blade templates.
In Livewire components, return$this->redirect()withnavigate: truefor SPA-like navigation.
In Mary UI modals, add a boolean property to the component class and usewire:modelin the Blade template.
Keep state server-side in Livewire components so the UI reflects it. Validate and authorize in actions as you would in HTTP requests.
Files:
app/Livewire/Configuration/Backup.php
**/*Resource.php
📄 CodeRabbit inference engine (CLAUDE.md)
For APIs, default to using Eloquent API Resources and API versioning unless existing API routes do not follow this pattern.
Files:
app/Http/Resources/OrganizationResource.php
🧠 Learnings (10)
📚 Learning: 2026-02-04T12:26:27.374Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 72
File: routes/web.php:66-81
Timestamp: 2026-02-04T12:26:27.374Z
Learning: In Laravel Livewire v4, prefer routing full-page Livewire components using Route::livewire('/path', ComponentClass::class) for all full-page components (whether class-based or view-based). While Route::get('/path', ComponentClass::class) remains technically functional for backward compatibility, it is not recommended in v4. Apply this guideline in all route files under routes (e.g., routes/web.php, routes/api.php) to ensure consistent, future-proof routing for Livewire components.
Applied to files:
routes/ai.phproutes/api.php
📚 Learning: 2026-02-25T10:48:17.811Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 132
File: app/Console/Commands/RecoverAgentLeasesCommand.php:44-48
Timestamp: 2026-02-25T10:48:17.811Z
Learning: When reviewing PHP code, especially with foreign keys that use cascadeOnDelete and are non-nullable, assume child relations exist at runtime (the database will delete children when the parent is deleted). Do not rely on null-safe operators for these relations, as PHPStan already models them as non-null. This guideline applies broadly to PHP files that define models with foreign keys using cascade delete; verify there are no unnecessary null checks or optional chaining on such relations.
Applied to files:
routes/ai.phpapp/Http/Controllers/Api/V1/UserOrganizationController.phpapp/Jobs/VerifySnapshotFileJob.phpconfig/oauth.phpapp/Mcp/Tools/ListOrganizationsTool.phpapp/Services/Backup/SnapshotVerificationService.phpapp/Livewire/Configuration/Backup.phpapp/Mcp/Servers/DatabasementServer.phproutes/api.phpapp/Services/OAuthService.phpapp/Http/Resources/OrganizationResource.php
📚 Learning: 2026-03-23T11:45:15.846Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 178
File: app/Http/Controllers/Api/V1/VolumeController.php:116-120
Timestamp: 2026-03-23T11:45:15.846Z
Learning: For the API v1 `testConnection` endpoints in this repo (e.g., `VolumeController::testConnection`, `DatabaseServerController::testConnection`), keep using `$this->authorize('view', $model)` for authorization. This is an intentional maintainer design choice; do not flag it as a bug or as insufficiently strict authorization in future reviews for these `testConnection` actions.
Applied to files:
app/Http/Controllers/Api/V1/UserOrganizationController.php
📚 Learning: 2026-05-05T22:58:34.407Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Http/Controllers/Api/V1/VolumeController.php:124-124
Timestamp: 2026-05-05T22:58:34.407Z
Learning: Context: In the databasement repository, the SetCurrentOrganization middleware is globally registered (bootstrap/app.php) so that for any authenticated user, CurrentOrganization::resolveForUser() returns an organization (user's first org or Organization::main() for super admins). Consequently, CurrentOrganization::id() will be non-null by the time an auth:sanctum-protected controller action runs. Review guidance: In Api controllers under app/Http/Controllers/Api, do not flag app(CurrentOrganization::class)->id() as potentially null for authenticated routes; however, keep null-checks for unauthenticated paths or other guards, and ensure tests cover API authentication flows. If this logic changes (e.g., middleware removal or different guards), re-evaluate.
Applied to files:
app/Http/Controllers/Api/V1/UserOrganizationController.php
📚 Learning: 2026-05-06T10:46:47.009Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Models/User.php:188-216
Timestamp: 2026-05-06T10:46:47.009Z
Learning: In app/Models/User.php, the $cachedRoles cache is intentionally request-scoped to prevent N+1 queries during rendering. Pivot writes (attach() in app/Livewire/User/Create.php, updateExistingPivot() in app/Livewire/Forms/UserForm.php) are terminal within a request, and the cache is not read after a write in the same request. Do not flag missing cache invalidation after pivot writes as a bug; this design is intentional. When reviewing related changes in PHP files under app/, assume this cache pattern and do not raise invalidation issues for these flows.
Applied to files:
app/Http/Controllers/Api/V1/UserOrganizationController.phpapp/Jobs/VerifySnapshotFileJob.phpapp/Mcp/Tools/ListOrganizationsTool.phpapp/Services/Backup/SnapshotVerificationService.phpapp/Livewire/Configuration/Backup.phpapp/Mcp/Servers/DatabasementServer.phpapp/Services/OAuthService.phpapp/Http/Resources/OrganizationResource.php
📚 Learning: 2026-05-05T23:16:24.464Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 275
File: app/Services/CurrentOrganization.php:28-39
Timestamp: 2026-05-05T23:16:24.464Z
Learning: Enforce non-nullable return types for id(): string and model(): Organization in PHP service classes under app/Services. Ensure SetCurrentOrganization middleware always resolves an Organization in the web context before these methods are reached; do not reintroduce isResolved guards in callers. A null dereference due to a broken middleware chain is the intended failure signal, not silent null propagation. Document the contract in class docblocks and consider tests to verify the middleware guarantees.
Applied to files:
app/Services/Backup/SnapshotVerificationService.phpapp/Services/OAuthService.php
📚 Learning: 2026-02-18T09:45:52.485Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 116
File: app/Livewire/DatabaseServer/ConnectionStatus.php:18-18
Timestamp: 2026-02-18T09:45:52.485Z
Learning: In Livewire components, Eloquent model properties (e.g., public DatabaseServer $server) are automatically locked by the framework to prevent client-side ID tampering. The #[Locked] attribute is only needed for scalar properties (int, string, bool, etc.) that require protection from client-side mutation. Apply this guidance to all Livewire PHP components; use #[Locked] only on primitive properties that you want to shield from client manipulation, and rely on automatic locking for Eloquent model properties.
Applied to files:
app/Livewire/Configuration/Backup.php
📚 Learning: 2026-02-01T11:14:33.775Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 64
File: docs/docs/self-hosting/configuration/application.md:186-189
Timestamp: 2026-02-01T11:14:33.775Z
Learning: In self-hosting documentation, prefer generic 'php artisan' command examples over Docker-specific commands to ensure guidance remains valid for users deploying via Docker, Kubernetes, native installations, or other methods. Apply this consistently across all files under docs/docs/self-hosting. If a Docker-specific command is necessary for illustration, provide a non-Docker alternative or clearly note the context and include an equivalent generic command.
Applied to files:
docs/docs/self-hosting/configuration/sso.md
📚 Learning: 2026-02-01T11:14:56.749Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 64
File: docs/docs/self-hosting/configuration/backup.md:141-144
Timestamp: 2026-02-01T11:14:56.749Z
Learning: In self-hosting documentation (docs/docs/self-hosting), prefer showing plain php artisan commands. Do not assume deployment environment. The Docker command (docker compose exec --user application -T app) is only relevant for local development and should not be included in deployment documentation.
Applied to files:
docs/docs/self-hosting/configuration/sso.md
📚 Learning: 2026-03-28T22:33:16.333Z
Learnt from: David-Crty
Repo: David-Crty/databasement PR: 195
File: app/Notifications/CLAUDE.md:7-12
Timestamp: 2026-03-28T22:33:16.333Z
Learning: In this repository (David-Crty/databasement), do not flag fenced Markdown code blocks that omit a language identifier (i.e., MD040: missing language after ```). The project treats unlabeled fenced code blocks as plain text by default, so missing language tags are acceptable for Markdown files.
Applied to files:
docs/docs/self-hosting/configuration/sso.mddocs/docs/user-guide/organizations.mddocs/docs/user-guide/permissions.md
🪛 LanguageTool
docs/docs/user-guide/permissions.md
[style] ~71-~71: Consider removing “of” to be more concise
Context: ...rg admins** can delete a user only when all of the following are true: - The target user ...
(ALL_OF_THE)
🪛 markdownlint-cli2 (0.22.1)
docs/docs/user-guide/organizations.md
[warning] 69-69: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 PHPMD (2.15.0)
app/Http/Resources/OrganizationResource.php
[warning] 19-19: Avoid unused parameters such as '$request'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (7)
app/Livewire/Configuration/Backup.php (1)
73-73: Org context is correctly passed into the verification job.Good change: dispatching with the active organization ID keeps verification scoped to the selected org.
app/Jobs/VerifySnapshotFileJob.php (1)
20-29: Constructor promotion and org forwarding look good.The job payload and handler now carry organization context cleanly into verification.
routes/ai.php (1)
9-10: LGTM —SetCurrentOrganizationcorrectly scopes org context for the web (HTTP) MCP transport.app/Http/Resources/OrganizationResource.php (1)
1-28: LGTM — Resource is well-structured. The PHPMDUnusedFormalParameterwarning on$requestis a false positive; the signature is mandated byJsonResource. The@phpstan-ignoreon the pivot property is the standard workaround for PHPStan's inability to modelwhenPivotLoadedpivot shapes.routes/api.php (1)
44-45: LGTM — Route is correctly placed inside theauth:sanctumgroup and consistently named (api.user.organizations). No org-scoping is needed for this endpoint since it returns all of the user's organizations by design.app/Http/Controllers/Api/V1/UserOrganizationController.php (1)
24-32: LGTM — Clean controller. The query is correctly scoped to the authenticated user and the endpoint intentionally returns all orgs (not just the active one), which is the right design for an org-discovery endpoint.app/Mcp/Servers/DatabasementServer.php (1)
16-31: LGTM — Tool registration and updated instructions correctly document org context switching.ListOrganizationsToolis listed first, which makes logical sense as the starting point for org discovery.
Introduce multi-organization support at the database level: - Add 3-step migration: create organizations/pivot tables, backfill existing data into a default organization, then finalize schema with non-nullable foreign keys and drop legacy role column - Add Organization model with membership management (users, resources, role checks, invitation links) - Add OrganizationScope global scope for automatic tenant filtering - Add OrganizationFactory with membership and admin state helpers - Extract user roles into UserRole enum (super_admin, admin, member) - Update DatabaseSeeder to create default organization with members
Wire up the runtime layer that makes multi-org work: - Add CurrentOrganization service (resolves org from session/cookie, manages context switching, provides org-aware query helpers) - Add SetCurrentOrganization middleware (sets org context per request, logs out users with no org membership) - Register middleware in bootstrap/app.php for all web/API routes - Update AppServiceProvider to bind CurrentOrganization as singleton and configure Fortify views with org context - Update FortifyServiceProvider for org-aware user creation - Update User model with organization relationships, role helpers, and membership management methods - Add organization_id + OrganizationScope to all tenant-scoped models (Agent, BackupJob, DatabaseServer, DatabaseServerSshConfig, Snapshot, Volume) - Update all model factories to include organization_id - Update test helpers (Pest.php, IntegrationTestHelpers) to set up organization context in tests
Add the UI layer for managing organizations: - Add Organization configuration page (create, edit, delete orgs; manage invitation links; view member counts and resource stats) - Add OrganizationSwitcher component in the app sidebar for switching between organizations the user belongs to - Add OrganizationPolicy for authorization (super admins manage all, org admins manage their own) - Update app layout to include org switcher and theme-init fixes - Add organization config tab and route - Add CSS for organization-related UI components - Add tests for organization CRUD and switcher functionality
Rework user management to be organization-aware: - Update UserPolicy with org-scoped permissions (org admins can manage users within their org, super admins manage all) - Update UserForm with organization and role selection fields - Update User Create to support org-specific invitation links, organization assignment, and role selection per org - Update User Edit with org-aware role editing - Update User Index with org-scoped user listing, removal vs deletion distinction, and improved filtering - Update all user management Blade views for new org fields - Update user management tests for multi-org scenarios
Adapt authentication and user settings for multi-org: - Update OAuthService to assign new OAuth users to an organization (via invitation link or default org) and set org context on login - Add oauth.php config for default organization assignment - Update Profile settings to show provider info for OAuth users - Update DeleteUserForm with org-aware deletion (password not required for OAuth users, info alerts about resource retention) - Update Password/TwoFactor settings with minor org context fixes - Update settings Blade views for OAuth user experience - Update auth tests (login, registration, OAuth) for org context - Update settings tests for org-aware assertions
Ensure all existing features respect the current organization context: - Update all dashboard components to scope queries to current org - Update BackupJobQuery and SnapshotQuery with org filtering - Update resource forms (Agent, DatabaseServer, Volume) to assign organization_id on creation - Update Configuration pages (Authentication, Backup) with org context - Update API controllers to scope resources by organization - Update DemoModeMiddleware, VerifySnapshotFileJob, and SnapshotVerificationService for org-aware operation - Update DemoBackupService to set org on demo data - Update all affected feature tests to set up organization context (agent, API, dashboard, database server, volume, config tests)
Expose organization data via API and MCP: - Add UserOrganizationController (list user's organizations) - Add OrganizationResource for API responses - Add ListOrganizationsTool for MCP (list orgs + switch context) - Register organization API routes and MCP tool - Add OrganizationContextApiTest (cross-org isolation, context switching via X-Organization-Id header) - Add UserOrganizationApiTest for the listing endpoint - Update MCP server tests for organization tool
- Add organizations user guide documentation - Update permissions docs with org admin role and scoping - Update SSO docs with organization assignment configuration - Update README with multi-tenant features and security reporting - Update SECURITY.md contact information - Add organization-related translations (fr, es) - Update docker-compose.yml service configuration - Update npm dependencies (package.json, package-lock.json) - Add custom Livewire pagination view (daisyUI themed) - Update API token index view for org context
e0da39c to
1c663cb
Compare
Always show delete and remove-from-org buttons for users the admin has authority over. When the action is blocked by a business rule, the confirmation modal explains why instead of hiding the button. - Move business rules (multi-org user, last super admin) from policies to component-level block reasons - Policies now only check authorization (role, org membership) - Delete modal warns when org admin targets a multi-org user - Remove modal warns when user only belongs to one organization - Server-side guards prevent bypassing the UI restrictions - Update dataset tests with three-outcome pattern (allowed/ forbidden/blocked)
Summary
Adds multi-organization (multi-tenant) support to Databasement. Each organization has its own isolated set of resources (database servers, volumes, agents, backup jobs, snapshots), with role-based access control at the organization level.
Key changes
CurrentOrganizationservice +SetCurrentOrganizationmiddleware automatically scope all queries to the active org via Eloquent global scopesUserRoleenum (super_admin,admin,member) replaces the old booleanis_admin— org admins manage their org, super admins manage everythingCommit breakdown
feat: add organization database schema, model, and core enums— migrations, Organization model, OrganizationScope, factory, UserRole enum, seederfeat: add multi-tenancy runtime infrastructure and model scoping— CurrentOrganization service, middleware, providers, User model, all model scopes, factories, test helpersfeat: add organization management UI and context switching— config page, switcher, policy, layout, routes, testsfeat: update user management for multi-organization support— UserPolicy, UserForm, CRUD components, views, testsfeat: update OAuth, auth flows, and user settings for organizations— OAuthService, settings components, auth/settings testsfeat: apply organization scope to existing features and update tests— dashboards, queries, forms, controllers, jobs, services, all affected testsfeat: add organization API endpoints and MCP tools— API controller, resource, MCP tool, routes, API/MCP testschore: update docs, translations, and infrastructure for multi-org— docs, i18n, docker, npm deps, pagination viewTest plan